[bug#38576] [PATCH] gnu: r-irkernel: Fix R kernel loading

[bug#38576] [PATCH] gnu: r-irkernel: Fix R kernel loading

[Lars-Dominik Braun]

* 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)))
@@ -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)))
-- 
2.20.1

[bug#38576] [PATCH] gnu: r-irkernel: Fix R kernel loading

[Ricardo Wurmus]

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

[bug#38576] [PATCH] gnu: r-irkernel: Fix R kernel loading

[Lars-Dominik Braun]

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

[bug#38576] [PATCH] gnu: r-irkernel: Fix R kernel loading

[zimoun]

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

[bug#38576] [PATCH] gnu: r-irkernel: Fix R kernel loading

[Ludovic Courtès]

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’.

[bug#38576] [PATCH] gnu: r-irkernel: Fix R kernel loading

[Lars-Dominik Braun]

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

[bug#38576] [PATCH] gnu: r-irkernel: Fix R kernel loading

[Ludovic Courtès]

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’.

bug#38576: [PATCH] gnu: r-irkernel: Fix R kernel loading

[Ludovic Courtès]

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’.

[bug#38576] [PATCH] gnu: r-irkernel: Fix R kernel loading

[zimoun]

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

[bug#38576] [PATCH] gnu: r-irkernel: Fix R kernel loading

[Roel Janssen]

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

[bug#38576] [PATCH] gnu: r-irkernel: Fix R kernel loading

[Ludovic Courtès]

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’.