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

  • Done
  • quality assurance status badge
Details
5 participants
  • Lars-Dominik Braun
  • Ludovic Courtès
  • Ricardo Wurmus
  • Roel Janssen
  • zimoun
Owner
unassigned
Submitted by
Lars-Dominik Braun
Severity
normal
L
L
Lars-Dominik Braun wrote on 12 Dec 2019 08:46
(address . guix-patches@gnu.org)
20191212074613.GA11713@zpidnp36
* 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(+)

Toggle diff (28 lines)
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
R
R
Ricardo Wurmus wrote on 12 Dec 2019 10:41
(name . Lars-Dominik Braun)(address . ldb@leibniz-psychology.org)(address . 38576@debbugs.gnu.org)
87wob13mpn.fsf@elephly.net
Hi Lars-Dominik,

Toggle quote (24 lines)
> * 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?

Toggle quote (10 lines)
> @@ -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
L
L
Lars-Dominik Braun wrote on 12 Dec 2019 11:04
(name . Ricardo Wurmus)(address . rekado@elephly.net)(address . 38576@debbugs.gnu.org)
20191212100452.GE22717@zpidnp36
Hi Ricardo,

Toggle quote (3 lines)
> 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.

Toggle quote (15 lines)
>
> > @@ -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
Z
Z
zimoun wrote on 12 Dec 2019 20:13
(name . Lars-Dominik Braun)(address . ldb@leibniz-psychology.org)
CAJ3okZ2SYmwZuJZARZnx7Ebigmh=usQdYCd-AEpy8PwDDoqPcg@mail.gmail.com
Hi,

Thank you for your patch!

On Thu, 12 Dec 2019 at 13:36, Lars-Dominik Braun
<ldb@leibniz-psychology.org> wrote:

Toggle quote (18 lines)
> > > @@ -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
L
L
Ludovic Courtès wrote on 19 Dec 2019 23:47
(name . Lars-Dominik Braun)(address . ldb@leibniz-psychology.org)
87r210j5kq.fsf@gnu.org
Hello,

Lars-Dominik Braun <ldb@leibniz-psychology.org> skribis:

Toggle quote (26 lines)
>> 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’.
L
L
Lars-Dominik Braun wrote on 2 Jan 2020 08:35
(name . Ludovic Courtès)(address . ludo@gnu.org)
20200102073536.GA3066@zpidnp36
Hi Ludo,

Toggle quote (2 lines)
> 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 ?

Toggle quote (3 lines)
> 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
L
L
Ludovic Courtès wrote on 2 Jan 2020 12:43
(name . Lars-Dominik Braun)(address . ldb@leibniz-psychology.org)
87h81ew0aw.fsf@gnu.org
Hi,

Lars-Dominik Braun <ldb@leibniz-psychology.org> skribis:

Toggle quote (5 lines)
>> 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.

Toggle quote (5 lines)
>> 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’.
L
L
Ludovic Courtès wrote on 3 Jan 2020 16:12
(name . Lars-Dominik Braun)(address . ldb@leibniz-psychology.org)
87lfqopo97.fsf@gnu.org
Ludovic Courtès <ludo@gnu.org> skribis:

Toggle quote (6 lines)
> 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’.
Closed
Z
Z
zimoun wrote on 6 Jan 2020 19:14
(name . Ludovic Courtès)(address . ludo@gnu.org)
CAJ3okZ0EryfOb77+X-5b6SfGsygs9ME6i3EnvT48KBk7r_VS0g@mail.gmail.com
Hi,

On Thu, 2 Jan 2020 at 12:44, Ludovic Courtès <ludo@gnu.org> wrote:

Toggle quote (4 lines)
> 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
R
R
Roel Janssen wrote on 7 Jan 2020 22:02
edd8dcb27c53f00df6f1305240f2bf3a564e5765.camel@gnu.org
On Thu, 2020-01-02 at 12:43 +0100, Ludovic Courtès wrote:
Toggle quote (27 lines)
> 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
L
L
Ludovic Courtès wrote on 8 Jan 2020 00:01
(name . Roel Janssen)(address . roel@gnu.org)
874kx6hnw9.fsf@gnu.org
Hi Roel,

Roel Janssen <roel@gnu.org> skribis:

Toggle quote (32 lines)
> 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’.
?