Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use JULIA_PROJECT env variable to activate julia env #612

Merged
merged 5 commits into from Mar 29, 2019

Conversation

@davidanthoff
Copy link
Contributor

davidanthoff commented Mar 6, 2019

Fixes #610.

This should be much more robust, plus is now also means that if one starts a julia at the terminal or something like that, all the packages from the environment are also available.

@NHDaly do you think this makes sense?

@davidanthoff davidanthoff reopened this Mar 6, 2019
@@ -118,7 +122,7 @@ def get_assemble_scripts(self):
(
"${NB_USER}",
r"""
julia -e "using Pkg; Pkg.add(\"IJulia\"); using IJulia; installkernel(\"Julia\", \"--project=${REPO_DIR}\", env=Dict(\"JUPYTER_DATA_DIR\"=>\"${NB_PYTHON_PREFIX}/share/jupyter\"));" && \
JULIA_PROJECT="" julia -e "using Pkg; Pkg.add(\"IJulia\"); using IJulia; installkernel(\"Julia\", env=Dict(\"JUPYTER_DATA_DIR\"=>\"${NB_PYTHON_PREFIX}/share/jupyter\"));" && \

This comment has been minimized.

Copy link
@betatim

betatim Mar 7, 2019

Member

For my education: Why set it to an empty value here? Not sure what I was expecting but explicitly setting it to be empty surprised me :)

This comment has been minimized.

Copy link
@NHDaly

NHDaly Mar 7, 2019

Contributor

I assume this is to make sure we install the default version of IJulia? But actually, that is a bit interesting... might we not also want to allow the user to specify a different version of IJulia if they so choose?

EDIT: after thinking more, this is probably actually so that we don't modify the user's environment, potentially breaking some reproducibility. We don't want to install anything into their environment, but we do need to install IJulia, and so we install it into the default environment. Since this PR is setting the project globally through the environment variable, we need to disable the setting so that julia will open w/ the default environment.

This comment has been minimized.

Copy link
@NHDaly

NHDaly Mar 7, 2019

Contributor

I wonder if we should activate and instantiate the user's project first, and then only add IJulia to the default environment if they don't already have IJulia installed? And only then install the kernel?

(Note that my above comments are orthogonal to this PR)

This comment has been minimized.

Copy link
@davidanthoff

davidanthoff Mar 8, 2019

Author Contributor

Yes, @NHDaly is correct: By setting this to "" we make sure the IJulia package gets installed into the default environment, not the user specified environment. Later, these environments are automatically stacked by julia. So if a user also had an IJulia version in their Project.toml, it would take precedence over the one we installed in the default environment.

I don't think we should install IJulia into the user provided env: that can potentially lead to updates of over, unrelated packages, and then we really break reproducability.

This comment has been minimized.

Copy link
@NHDaly

NHDaly Mar 8, 2019

Contributor

I don't think we should install IJulia into the user provided env: that can potentially lead to updates of over, unrelated packages, and then we really break reproducability.

Yeah, I had the same concern. I agree with you here 100%!

So if a user also had an IJulia version in their Project.toml, it would take precedence over the one we installed in the default environment.

But what i was concerned about is that if a user specifies a different version of IJulia in their Project.toml, we're still going to install the kernel from this default version, not the one they specify. Would it be crazy to peek ahead and only install IJulia into the default env if the user doesn't include it in theirs? (Either way, this seems like a feature that could be added in a later PR)

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Mar 7, 2019

I think my only question (as a none Julia user) is: In what ways could we be breaking users with this change? Should we add a FAQ entry to help them or even define both JULIA_BINDIR and JULIA_PROJECT for "a few releases" to allow a transition?

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Mar 7, 2019

Hmm, i'm not sure if this makes sense. And in fact, I actually didn't notice that you added this installkernel(\"Julia\", \"--project=${REPO_DIR}\", section in #595. It's cool! But sorry i didn't comment on it when reviewing.

On the one hand, I like the spirit of it: Yeah, you're using the Project.toml to specify what your environment should look like, so yeah you'd want that environment to be activated when you run notebooks! It will be nice not to have to run Pkg.activate(".") at the start of every notebook.

But it is a bit unexpected to me because it's not how the default IJulia kernel behaves. So if I clone this repo and run jupyter from there locally, my notebook won't work, even though the repo has the Project.toml file in it... I would need to add using Pkg; Pkg.activate("."). So I kind of feel like maybe asking the user to activate the project isn't unreasonable, at least for reproducibility reasons?

I do agree that it would be nice to avoid the boilerplate activate("."), but I don't know that the tradeoff is worth it. What do you think?


That said, I think I share your feeling that setting this environment variable is better than setting that flag in the Jupyter Kernel, but I'm not very sure. I don't think we'd want the shell to open into the default environment, since presumably users would be using the shell to modify things about the notebooks.

One case that may be harmed would be if they wanted to make changes to the IJulia installation itself; it's difficult to reactivate the default environment once you've opened Julia with this flag set, and people may not know why that's happening, since I don't think this flag is commonly used. I could imagine people typing ]activate into the REPL, surprised that it doesn't drop them out of the project. (That certainly surprised me in the past.)

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Mar 7, 2019

(After some investigation, i've just discovered that you can reactivate the default environment via pkg> activate --shared v1.1, but I don't think most people know about that.)

@davidanthoff

This comment has been minimized.

Copy link
Contributor Author

davidanthoff commented Mar 8, 2019

In what ways could we be breaking users with this change?

I think none, given that this only affects the Project.toml story, which hasn't really shipped? Or at least not for more than 2 days ;)

But it is a bit unexpected to me because it's not how the default IJulia kernel behaves.

Yes, that is a good point. Lets see whether we can change the IJulia default: JuliaLang/IJulia.jl#820.

I think at this point the main question is whether we want to activate the env in the Project.toml automatically or not. Right now on master, we do. I think this PR here keeps that behavior but implements it in a more elegant way. I can't really think of any downside of this approach vs what we have on master, to be honest...

So I think maybe we can merge this here, but then also keep the option open to actually not activate the environment for the notebook at all, depending on how the discussion over in the IJulia issue turns out?

I guess another question is how this works for other languages in binder? I assume that if there is a requirements.txt file in the root, and then you run a notebook in that repo, the packages from that are just available, and you don't have to somehow activate things in some way?

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Mar 8, 2019

Lets see whether we can change the IJulia default: JuliaLang/IJulia.jl#820.

Yes! I love the idea of doing this by default in other kernels as well. Great idea, @davidanthoff! :) +1

I think at this point the main question is whether we want to activate the env in the Project.toml automatically or not. Right now on master, we do. I think this PR here keeps that behavior but implements it in a more elegant way. I can't really think of any downside of this approach vs what we have on master, to be honest...

The only potential downside I could see is that this PR enables the same behavior for julia shells opened in the terminal, which is also a bit unexpected and different from the normal julia behavior. I don't feel strongly one or the other, but my preference would be towards only doing this in the notebooks, not the shell, via the flag, as things already are on master.


One more question: is REPO_DIR the destination after everything is copied? I don't remember. I just want to make sure that if the user installs more packages via the shell, or manually modifies the Project.toml file that is next to the notebooks, those changes will be reflected (after a pkg"instantiate") right?

@davidanthoff

This comment has been minimized.

Copy link
Contributor Author

davidanthoff commented Mar 14, 2019

So an update on this one: IJulia now merged this behavior, so for Jupyter we should be in the clear with auto activating the environment.

So the question now is whether we also want to auto-activate the environment for any other julia process, i.e. a non-IJulia kernel, or not. I think we should, actually :) Isn't the whole point of binder that you get a pre-populated environment?

One question that might be good to sort out is how users can actually start a julia process other than an IJulia kernel? I'm just not familiar enough with the binder setup, maybe @betatim can shed some light on this? Can one e.g. SSH into a binder instance?

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Mar 14, 2019

This might be a silly question, but now that IJulia has merged this behavior, can we just rely on IJulia to do it by default, instead of implementing it ourselves?


One question that might be good to sort out is how users can actually start a julia process other than an IJulia kernel? I'm just not familiar enough with the binder setup, maybe @betatim can shed some light on this? Can one e.g. SSH into a binder instance?

One way that I can think of:
Binder just creates a normal Jupyter notebook server, and the Jupyter notebook UI has the ability to launch a terminal (in the browser) that's running on the host. It's essentially like SSHing into the instance, but accessing it through an in-browser remote terminal UI instead.
You can get there by clicking new->terminal:
Screen Shot 2019-03-14 at 3 10 47 PM

And it looks like this:
Screen Shot 2019-03-14 at 3 10 54 PM
(For me the link was: https://hub.mybinder.org/user/nhdaly-jupyter-binder-040a6vbx/terminals/1, but that seems to be instance-specific.)

So the question is, if we open the Julia REPL through that terminal, should it activate your environment?


My gut is to vote No, since that's not how the terminal behaves on your local computer. And anyone who's managed to put a Project.toml file in their repo knows how to activate a directory. But I do also see how it makes sense from a usability perspective: why would you ever NOT want your project activated? Also, since this docker instance is short lived, and about to be killed, I can't see any harm in having your project activated when you didn't expect it. You're not going to break anything!

SO I guess what i'm saying is yeah I think your change makes sense, @davidanthoff. :) I've talked myself into it, and now I vote Yes!

@NHDaly
NHDaly approved these changes Mar 19, 2019
Copy link
Contributor

NHDaly left a comment

Okay so to clarify my earlier comment: I think this is a good idea!

I approve, and I think we should merge it. :) Haha sorry it took me a while to come around.

@davidanthoff

This comment has been minimized.

Copy link
Contributor Author

davidanthoff commented Mar 19, 2019

I think we should now first merge #622, and then I can resolve any conflicts between that and this PR here, and then we can merge this PR here.

@davidanthoff davidanthoff changed the title Use JULIA_PROJECT env variable to activate julia env WIP Use JULIA_PROJECT env variable to activate julia env Mar 19, 2019
@davidanthoff davidanthoff changed the title WIP Use JULIA_PROJECT env variable to activate julia env Use JULIA_PROJECT env variable to activate julia env Mar 21, 2019
@davidanthoff

This comment has been minimized.

Copy link
Contributor Author

davidanthoff commented Mar 21, 2019

This is ready from my end. @NHDaly do you want to review this final version?

I also removed JULIA_BINDIR in this PR because it really is a julia 0.6 thing that serves no purpose for the Project.toml based story.

@NHDaly
NHDaly approved these changes Mar 25, 2019
Copy link
Contributor

NHDaly left a comment

LGTM! Thanks @davidanthoff! :)

@davidanthoff

This comment has been minimized.

Copy link
Contributor Author

davidanthoff commented Mar 25, 2019

Great, thanks! @betatim ready for your review/merge!

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Mar 29, 2019

Thanks! LGTM -> merged.

@betatim betatim merged commit 053a623 into jupyter:master Mar 29, 2019
5 checks passed
5 checks passed
ci/circleci: build_docs Your tests passed on CircleCI!
Details
ci/dockercloud Your tests passed in Docker Cloud
Details
codecov/patch 100% of diff hit (target 20%)
Details
codecov/project 90.77% (+0.01%) compared to e921993
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@davidanthoff davidanthoff deleted the davidanthoff:change-julia-env-handling branch Mar 29, 2019
@davidanthoff

This comment has been minimized.

Copy link
Contributor Author

davidanthoff commented Mar 29, 2019

Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.