[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’.
?
Your comment

This issue is archived.

To comment on this conversation send an email to 38576@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 38576
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch