Add special env to take r shadowing functions #324

jankatins · 2016-05-11T17:56:47Z

Closes: #301

We need to get some functions into the search path so that they shadow functions of R proper. One examples is `quit()` which we have to implement so that it properly shuts down the kernel and tells the frontend that it wants to shutdown. After some experimentation, the environment had to be added in the main codepath so that a) other places in the code could access it and b) it was actually places into the searchpath between the already loaded packages and the .Globalenv.

jankatins · 2016-05-11T18:09:37Z


And that's how it looks like:

c:\data\external\R\IRkernel (envs)
λ jupyter console --kernel ir33
Jupyter Console 4.1.1


In [1]: search()
 [1] ".GlobalEnv"        "jupyter:irkernel"  "package:stats"
 [4] "package:graphics"  "package:grDevices" "package:utils"
 [7] "package:datasets"  "package:methods"   "Autoloads"
[10] "package:base"

flying-sheep · 2016-05-11T20:22:20Z

R/kernel.r

+    on.exit({
+        detach("jupyter:irkernel")
+    })
+    assign(".irk.get_userenv", function() {.irk.env})


where does this one end up?

In .irk.env

Or it should, will fix...

jankatins · 2016-05-11T21:45:28Z

Latest code:

c:\data\external\R
λ jupyter console --kernel ir33
Jupyter Console 4.1.1


In [1]: print("x")
[1] "x"

In [2]: .ir
.irk.add_to_user_searchpath .irk.get_userenv

In [2]: .irk.get_userenv()
<environment: 0x000000000747cc78>
attr(,"name")
[1] "jupyter:irkernel"

In [3]: names(.irk.get_userenv())
[1] "quit"                        ".irk.get_userenv"
[3] ".irk.add_to_user_searchpath" "q"

In [4]: search
function ()
.Internal(search())
<bytecode: 0x0000000006d5d2f8>
<environment: namespace:base>

In [5]: search()
 [1] ".GlobalEnv"        "jupyter:irkernel"  "package:stats"
 [4] "package:graphics"  "package:grDevices" "package:utils"
 [7] "package:datasets"  "package:methods"   "Autoloads"
[10] "package:base"

In [6]: q()
Shutting down kernel

c:\data\external\R
λ

jankatins · 2016-05-11T21:47:55Z

I think I have now created and reverted environment.r three times and each time I couldn't get it to work...

flying-sheep · 2016-05-12T08:22:05Z

OK, what didn’t work?

on.exit has of course to be inside of main because it triggers when the enclosing function returns
did you try putting everything except on.exit into a function and calling that one in main?
did you try getting the env by name every time you use it? i.e. create a global env.name <- 'jupyter:irkernel' and then do assign('name', thing, env.name)? then you can maybe do it in a separate file.

after all, it would be best to have all the stuff in environment.r as you tried, and the assign calls in main

takluyver · 2016-05-12T09:32:02Z

R/kernel.r

+    on.exit({
+        detach("jupyter:irkernel")
+    })
+    assign(".irk.get_userenv", function() {.irk.env}, .irk.env)


Naming quibble: 'user environment' is a lot like 'user namespace', which in IPython means the namespace where user code executes (i.e. typing a = 1 defines a in the user namespace). That's not what we mean here. Can we come up with a clearer name? E.g. get_shadowing_env?

jankatins · 2016-05-12T10:00:03Z

OK, what didn’t work?

implementing it as a "package environment" much like the one for the com manager and on.exit also on that level (though of it as "when the package is unloaded"). But putting it there, the code was never executing (surrounded it with prints) and search() never showed anything
Creating the env manually via new.env() and only attaching it there, but having the rest of the functions defined elsewhere -> attach copies the env, so the functions in that place still had a reference to the old env, but that env was actually not in the search path but the copy... Was funny because .irk.get_userenv() was avaiölable (was copied into the new env) and returned the old env...

What could work is

define the .irk.shadowenv variable in environment.r and set initially to NULL
define the functions as normal functions (-> can then only be used in the kernel package)
add a init_shadowenv() functions which adds these functions to the shadow env
then add .irk.shadowenv <<- attach(null, name='jupyter:irkernel') (+ on.exit) in main().

Will try that now...

jankatins · 2016-05-12T10:14:48Z

Ah, and that doesn't work because of

Error in init_shadowenv() :
  cannot change value of locked binding for '.irk.shadowenv'
Calls: <Anonymous> -> init_shadowenv

If someone wants to have a try, this is the best I can give you... :-/

For reference:

# Everthing related to the environment which takes functions which shadow base R functions.
# This is needed to build in our own needs, like properly shutting down the kernel 
# when `quit()` is called.

# The real env is created with attach in the main() via the call to init_shadowenv()!
.irk.shadowenv <- NULL

get_shadowenv <- function() {.irk.shadowenv}

add_to_user_searchpath <- function(name, FN, attrs = list()) {
    if (is.null(.irk.shadowenv)) stop("ASSERT: shadowenv was NULL", call. = FALSE)
    assign(name, FN, .irk.shadowenv)
}

init_shadowenv <- function(){
    .irk.shadowenv <<- attach(NULL, name = "jupyter:irkernel")
    # also add the accessors to the shadow env to that env
    assign(".irk.get_shadowenv", get_shadowenv, .irk.shadowenv)
    assign(".irk.add_to_user_searchpath", add_to_user_searchpath, .irk.shadowenv)
}

and then in kernel.r -> main():

init_shadowenv()
    on.exit({
        detach("jupyter:irkernel")
    })

flying-sheep · 2016-05-12T10:27:38Z

you missed my point about using the env name and not the env object. that will probably work

jankatins · 2016-05-12T10:28:25Z

Ok, I found a way to put it all into a function and call it from main... It's also not as nice, but at least all the code is in one place...

jankatins · 2016-05-12T10:37:21Z

So, have a look and merge, I think this is ready...

takluyver · 2016-05-12T10:43:12Z

Seems reasonable.

The env and the functions to access them have to be done in the function which is called from main() because: * Attaching it on the package level does not add it to the search path during evaluate * creating it on the package level and only attaching it in main() will copy the env and we can#t get a handle to that copied env * reassigning the env to the package level variable needs some trickery (it's locked) and that seems worse than doing it in a big function...

flying-sheep · 2016-05-12T12:28:57Z

R/environment.r

+
+# The real env is created and attach'ed in the main() via the call to init_shadowenv()!
+init_shadowenv <- function(){
+    .irk.shadowenv <- attach(NULL, name = "jupyter:irkernel")


){ → ) {

"" → ''

I should really invest some time into setting up some precommit hook...

i tried to create some lintr rules, but somehow can’t get it to run: r-lib/lintr#139

no idea what i do wrong

flying-sheep · 2016-05-12T12:35:28Z

jup, looks good! some questions and style things remain.

jankatins · 2016-05-12T21:18:04Z

Refactored again: now the thing is out of main() and in the executor.

Also use names instead of direct env references. All additional functions which should be laced into the shadow env need to be added in the `init_shadowenv()` function.

flying-sheep · 2016-05-13T12:48:41Z

perfect!

jankatins added 2 commits May 11, 2016 19:44

More R CMD check fixes/comments

7e10b4b

jankatins mentioned this pull request May 11, 2016

License... #322

Closed

flying-sheep reviewed May 11, 2016
View reviewed changes

jankatins added 2 commits May 11, 2016 23:45

Fix assigning functions to the new shadow env

0800799

Add myself as author

e1a9579

takluyver reviewed May 12, 2016
View reviewed changes

Rename to shadowenv to avoid name confusion

7e2265f

jankatins force-pushed the envs branch 2 times, most recently from 4073d36 to c770890 Compare May 12, 2016 10:37

jankatins force-pushed the envs branch from c770890 to f9a9ba4 Compare May 12, 2016 10:42

jankatins force-pushed the envs branch from f9a9ba4 to d02847d Compare May 12, 2016 10:46

flying-sheep reviewed May 12, 2016
View reviewed changes

Add creation and detachment of shadow env to executor

a081831

Also use names instead of direct env references. All additional functions which should be laced into the shadow env need to be added in the `init_shadowenv()` function.

jankatins force-pushed the envs branch from 2a92058 to a081831 Compare May 12, 2016 21:21

flying-sheep merged commit 9d15ecc into IRkernel:master May 13, 2016

flying-sheep mentioned this pull request May 13, 2016

More tests #330

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add special env to take r shadowing functions #324

Add special env to take r shadowing functions #324

jankatins commented May 11, 2016

jankatins commented May 11, 2016

flying-sheep May 11, 2016

jankatins May 11, 2016

jankatins May 11, 2016

flying-sheep May 12, 2016 •

edited

jankatins commented May 11, 2016

jankatins commented May 11, 2016

flying-sheep commented May 12, 2016 •

edited

takluyver May 12, 2016

jankatins May 12, 2016

jankatins commented May 12, 2016 •

edited

jankatins commented May 12, 2016 •

edited

flying-sheep commented May 12, 2016

jankatins commented May 12, 2016

jankatins commented May 12, 2016

takluyver commented May 12, 2016

flying-sheep May 12, 2016 •

edited

jankatins May 12, 2016

flying-sheep May 12, 2016 •

edited

flying-sheep commented May 12, 2016

jankatins commented May 12, 2016 •

edited

flying-sheep commented May 13, 2016

Add special env to take r shadowing functions #324

Add special env to take r shadowing functions #324

Conversation

jankatins commented May 11, 2016

jankatins commented May 11, 2016

flying-sheep May 11, 2016

Choose a reason for hiding this comment

jankatins May 11, 2016

Choose a reason for hiding this comment

jankatins May 11, 2016

Choose a reason for hiding this comment

flying-sheep May 12, 2016 • edited

Choose a reason for hiding this comment

jankatins commented May 11, 2016

jankatins commented May 11, 2016

flying-sheep commented May 12, 2016 • edited

takluyver May 12, 2016

Choose a reason for hiding this comment

jankatins May 12, 2016

Choose a reason for hiding this comment

jankatins commented May 12, 2016 • edited

jankatins commented May 12, 2016 • edited

flying-sheep commented May 12, 2016

jankatins commented May 12, 2016

jankatins commented May 12, 2016

takluyver commented May 12, 2016

flying-sheep May 12, 2016 • edited

Choose a reason for hiding this comment

jankatins May 12, 2016

Choose a reason for hiding this comment

flying-sheep May 12, 2016 • edited

Choose a reason for hiding this comment

flying-sheep commented May 12, 2016

jankatins commented May 12, 2016 • edited

flying-sheep commented May 13, 2016

flying-sheep May 12, 2016 •

edited

flying-sheep commented May 12, 2016 •

edited

jankatins commented May 12, 2016 •

edited

jankatins commented May 12, 2016 •

edited

flying-sheep May 12, 2016 •

edited

flying-sheep May 12, 2016 •

edited

jankatins commented May 12, 2016 •

edited