Message ID | 20191212074613.GA11713@zpidnp36 |
---|---|
State | Accepted |
Headers | show |
Series | [bug#38576] gnu: r-irkernel: Fix R kernel loading | expand |
Hi Lars-Dominik, > * gnu/packages/cran.scm (r-irkernel): Absolute path to R binary > [propagated-inputs]: Generate proper search paths by adding r-minimal > --- > gnu/packages/cran.scm | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/gnu/packages/cran.scm b/gnu/packages/cran.scm > index 765747ea3b..c54a076014 100644 > --- a/gnu/packages/cran.scm > +++ b/gnu/packages/cran.scm > @@ -12414,6 +12414,12 @@ running IRkernel session.") > "--name" "ir" > "--prefix" out > (string-append out "/site-library/IRkernel/kernelspec")) > + ;; Record the absolute file name of the 'R' executable in > + ;; 'kernel.json'. > + (substitute* (string-append out "/share/jupyter" > + "/kernels/ir/kernel.json") > + (("\\[\"R\",") > + (string-append "[\"" (which "R") "\","))) > #t)))))) > (inputs > `(("jupyter" ,jupyter))) This part looks fine to me, though I wonder if that’s what users of this package would expect. Is there an expectation that the effective R is defined by the environment? Or would that not work anyway? > @@ -12423,6 +12429,8 @@ running IRkernel session.") > ("r-evaluate" ,r-evaluate) > ("r-irdisplay" ,r-irdisplay) > ("r-jsonlite" ,r-jsonlite) > + ;; sets R_LIBS_SITE, so R can actually find this package (IRkernel) > + ("r-minimal" ,r-minimal) > ("r-pbdzmq" ,r-pbdzmq) > ("r-repr" ,r-repr) > ("r-uuid" ,r-uuid))) This doesn’t look right to me. It seems wrong for any R package to propagate R itself. The R_LIBS_SITE variable is “attached” to “r-minimal”, so when that is installed R will find the r-irkernel package. Am I missing something? -- Ricardo
Hi Ricardo, > This part looks fine to me, though I wonder if that’s what users of this > package would expect. Is there an expectation that the effective R is > defined by the environment? Or would that not work anyway? it’s what python-ipykernel does – without explanation though. I’m not an R expert, so I’m unsure whether any R installation from the environment (which could be user-installed in $HOME) would be able to load this plugin or just the one it was “built” for. This change assumes the latter. > > > @@ -12423,6 +12429,8 @@ running IRkernel session.") > > ("r-evaluate" ,r-evaluate) > > ("r-irdisplay" ,r-irdisplay) > > ("r-jsonlite" ,r-jsonlite) > > + ;; sets R_LIBS_SITE, so R can actually find this package (IRkernel) > > + ("r-minimal" ,r-minimal) > > ("r-pbdzmq" ,r-pbdzmq) > > ("r-repr" ,r-repr) > > ("r-uuid" ,r-uuid))) > > This doesn’t look right to me. It seems wrong for any R package to > propagate R itself. The R_LIBS_SITE variable is “attached” to > “r-minimal”, so when that is installed R will find the r-irkernel > package. Am I missing something? If r-minimal is not installed, the kernel will simply not work and thus render this package useless. That’s why I would consider it a dependency. Cheers, Lars
Hi, Thank you for your patch! On Thu, 12 Dec 2019 at 13:36, Lars-Dominik Braun <ldb@leibniz-psychology.org> wrote: > > > @@ -12423,6 +12429,8 @@ running IRkernel session.") > > > ("r-evaluate" ,r-evaluate) > > > ("r-irdisplay" ,r-irdisplay) > > > ("r-jsonlite" ,r-jsonlite) > > > + ;; sets R_LIBS_SITE, so R can actually find this package (IRkernel) > > > + ("r-minimal" ,r-minimal) > > > ("r-pbdzmq" ,r-pbdzmq) > > > ("r-repr" ,r-repr) > > > ("r-uuid" ,r-uuid))) > > > > This doesn’t look right to me. It seems wrong for any R package to > > propagate R itself. The R_LIBS_SITE variable is “attached” to > > “r-minimal”, so when that is installed R will find the r-irkernel > > package. Am I missing something? > If r-minimal is not installed, the kernel will simply not work and thus > render this package useless. That’s why I would consider it a dependency. Hum? it is true for any R package, isn't it? I mean, all the R packages needs R to work but R is not a dependency. We can discuss if a R package has to propagate R itself or not. But it is not how the R packages are usually defined in Guix; AFAIU. To stay coherent, "r-minimal" should not be propagated or I also miss something. :-) Currently, I usually type "guix install r r-pkg" and not "guix install r-pkg". All the best, simon
Hello, Lars-Dominik Braun <ldb@leibniz-psychology.org> skribis: >> This part looks fine to me, though I wonder if that’s what users of this >> package would expect. Is there an expectation that the effective R is >> defined by the environment? Or would that not work anyway? > it’s what python-ipykernel does – without explanation though. I’m not an R > expert, so I’m unsure whether any R installation from the environment (which > could be user-installed in $HOME) would be able to load this plugin or just the > one it was “built” for. This change assumes the latter. > >> >> > @@ -12423,6 +12429,8 @@ running IRkernel session.") >> > ("r-evaluate" ,r-evaluate) >> > ("r-irdisplay" ,r-irdisplay) >> > ("r-jsonlite" ,r-jsonlite) >> > + ;; sets R_LIBS_SITE, so R can actually find this package (IRkernel) >> > + ("r-minimal" ,r-minimal) >> > ("r-pbdzmq" ,r-pbdzmq) >> > ("r-repr" ,r-repr) >> > ("r-uuid" ,r-uuid))) >> >> This doesn’t look right to me. It seems wrong for any R package to >> propagate R itself. The R_LIBS_SITE variable is “attached” to >> “r-minimal”, so when that is installed R will find the r-irkernel >> package. Am I missing something? > If r-minimal is not installed, the kernel will simply not work and thus > render this package useless. That’s why I would consider it a dependency. An argument in favor of the status quo would be that it allows users to choose between ‘r’ and ‘r-minimal’. Is that a compelling argument? It may be more important for ‘r-irkernel’ to work out of the box, like you did. However, if we go that route, we should arrange to not propagate ‘r-minimal’ (it’s intrusive) and instead have ‘kernel.json’ do the right thing. How does that sound? Thanks, Ludo’.
Hi Ludo, > An argument in favor of the status quo would be that it allows users to > choose between ‘r’ and ‘r-minimal’. Is that a compelling argument? reading the documentation I thought this was possible using --with-input=r-minimal=r ? > However, if we go that route, we should arrange to not propagate > ‘r-minimal’ (it’s intrusive) and instead have ‘kernel.json’ do the right > thing. I’m not following, sorry. What do you suggest kernel.json should do? Lars
Hi, Lars-Dominik Braun <ldb@leibniz-psychology.org> skribis: >> An argument in favor of the status quo would be that it allows users to >> choose between ‘r’ and ‘r-minimal’. Is that a compelling argument? > reading the documentation I thought this was possible using > --with-input=r-minimal=r ? Yes, good point. >> However, if we go that route, we should arrange to not propagate >> ‘r-minimal’ (it’s intrusive) and instead have ‘kernel.json’ do the right >> thing. > I’m not following, sorry. What do you suggest kernel.json should do? I was suggesting hard-coding the file name of the ‘R’ executable in ‘kernel.json’, but I see you already did that in your initial patch. Sorry for the confusion. On second thought, I think propagating R is acceptable in this case because a Jupyter kernel is a thin wrapper around a programming language implementation. Unless there are objections, I’ll apply your initial patch. Apologies for the delay, but I think it was good to have this discussion! Thanks, Ludo’.
Ludovic Courtès <ludo@gnu.org> skribis: > On second thought, I think propagating R is acceptable in this case > because a Jupyter kernel is a thin wrapper around a programming language > implementation. > > Unless there are objections, I’ll apply your initial patch. Done! Ludo’.
Hi, On Thu, 2 Jan 2020 at 12:44, Ludovic Courtès <ludo@gnu.org> wrote: > On second thought, I think propagating R is acceptable in this case > because a Jupyter kernel is a thin wrapper around a programming language > implementation. So, the argument is "IRkernal is not an ordinary R package" and because this very package is special it "is acceptable" to propagate R (r-minimal). No objection, but adding this explanation or a link to this thread appears to me welcome. :-) For example in the commit message or perhaps better a comment in propagated-inputs. Thank you for this package. Nice to have it! :-) All the best, simon
On Thu, 2020-01-02 at 12:43 +0100, Ludovic Courtès wrote: > Hi, > > Lars-Dominik Braun <ldb@leibniz-psychology.org> skribis: > > > > An argument in favor of the status quo would be that it allows users to > > > choose between ‘r’ and ‘r-minimal’. Is that a compelling argument? > > reading the documentation I thought this was possible using > > --with-input=r-minimal=r ? > > Yes, good point. > > > > However, if we go that route, we should arrange to not propagate > > > ‘r-minimal’ (it’s intrusive) and instead have ‘kernel.json’ do the right > > > thing. > > I’m not following, sorry. What do you suggest kernel.json should do? > > I was suggesting hard-coding the file name of the ‘R’ executable in > ‘kernel.json’, but I see you already did that in your initial patch. > Sorry for the confusion. > > On second thought, I think propagating R is acceptable in this case > because a Jupyter kernel is a thin wrapper around a programming language > implementation. > > Unless there are objections, I’ll apply your initial patch. > I'm too late, but doesn't this break the kernel for people who have the "full" R in their profile, and therefore expect the "full" R to be available in a Jupyter notebook? Kind regards, Roel Janssen
Hi Roel, Roel Janssen <roel@gnu.org> skribis: > On Thu, 2020-01-02 at 12:43 +0100, Ludovic Courtès wrote: >> Hi, >> >> Lars-Dominik Braun <ldb@leibniz-psychology.org> skribis: >> >> > > An argument in favor of the status quo would be that it allows users to >> > > choose between ‘r’ and ‘r-minimal’. Is that a compelling argument? >> > reading the documentation I thought this was possible using >> > --with-input=r-minimal=r ? >> >> Yes, good point. >> >> > > However, if we go that route, we should arrange to not propagate >> > > ‘r-minimal’ (it’s intrusive) and instead have ‘kernel.json’ do the right >> > > thing. >> > I’m not following, sorry. What do you suggest kernel.json should do? >> >> I was suggesting hard-coding the file name of the ‘R’ executable in >> ‘kernel.json’, but I see you already did that in your initial patch. >> Sorry for the confusion. >> >> On second thought, I think propagating R is acceptable in this case >> because a Jupyter kernel is a thin wrapper around a programming language >> implementation. >> >> Unless there are objections, I’ll apply your initial patch. >> > > I'm too late, but doesn't this break the kernel for people who have the "full" R > in their profile, and therefore expect the "full" R to be available in a Jupyter > notebook? Indeed, it forces ‘r-minimal’ for use in the kernel. But there are workarounds, as Lars-Dominik suggests above. WDYT? Ludo’.
diff --git a/gnu/packages/cran.scm b/gnu/packages/cran.scm index 765747ea3b..c54a076014 100644 --- a/gnu/packages/cran.scm +++ b/gnu/packages/cran.scm @@ -12414,6 +12414,12 @@ running IRkernel session.") "--name" "ir" "--prefix" out (string-append out "/site-library/IRkernel/kernelspec")) + ;; Record the absolute file name of the 'R' executable in + ;; 'kernel.json'. + (substitute* (string-append out "/share/jupyter" + "/kernels/ir/kernel.json") + (("\\[\"R\",") + (string-append "[\"" (which "R") "\","))) #t)))))) (inputs `(("jupyter" ,jupyter))) @@ -12423,6 +12429,8 @@ running IRkernel session.") ("r-evaluate" ,r-evaluate) ("r-irdisplay" ,r-irdisplay) ("r-jsonlite" ,r-jsonlite) + ;; sets R_LIBS_SITE, so R can actually find this package (IRkernel) + ("r-minimal" ,r-minimal) ("r-pbdzmq" ,r-pbdzmq) ("r-repr" ,r-repr) ("r-uuid" ,r-uuid)))