-
-
Notifications
You must be signed in to change notification settings - Fork 286
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
implement per notebook env in backend #341
Conversation
@fonsp any comments on this? |
src/evaluation/WorkspaceManager.jl
Outdated
if isempty(project) | ||
project = default_env() | ||
end | ||
# NOTE: notebook environment should not be the same as server process environment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting! Why not?
Should we check for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I might be too brief in the note, I wrote this to explain why I force all workspace process to start with a --project
flag so it will never inherit server process environment implicitly.
I think this behaviour is more natural, since in principal we don't assume different notebook shares the same environment at the backend (unless it is declared explicitly in the frontend).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other solution is to inherit the environment if project === nothing
is true
. But I find this is not something I'd intuitively expect when I work with notebook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it currently inherit the environment? That's not good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean master
branch or this PR?
- it inherits from the environment in
master
- it does not inherit in this PR
I was just wondering if I should preserve master
branch behaviour (since it might break things if not), but then I realize it's not intuitive to inherit the environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No that was unintended! It should not inherit - thanks for spotting the mistake
src/notebook/Notebook.jl
Outdated
@@ -29,11 +29,13 @@ mutable struct Notebook | |||
pendingupdates::Channel | |||
|
|||
executetoken::Token | |||
|
|||
project::String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is more a comment about Julia but can we use a longer name?
Is project_environment_path
appropriate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, project_environment_path
sounds better
src/Pluto.jl
Outdated
@@ -26,6 +26,17 @@ const ENV_DEFAULTS = Dict( | |||
joinpath(preferred_dir, "") # must end with / or \ | |||
end, | |||
) | |||
|
|||
function default_env() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pluto already has a default ENV system that we can use, get_pl_env
. This means that we only use the ENV
name "PLUTO_PROJECT"
(although see my later comment about the name).
Since we are making the naming decision right now, we should just pick a single ENV name instead of two different ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's a convention that I should have used "JULIA_PLUTO_*"
for all ENV names, then I'm okay with changing the names of all current pluto ENV variables. It's undocumented and probably not used by anyone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the reason is we should provide environment variable starts with both PLUTO
and JULIA_PLUTO
just in case. Since this name PLUTO
could be used by other things too. What do you think?
Pluto already has a default ENV system that we can use, get_pl_env. This means that we only use the ENV name "PLUTO_PROJECT" (although see my later comment about the name).
I could update this too, since I'm also a bit concerned about the way it is implemented: current implementation will break the ability to create a distributable binary from Pluto using PackageCompiler, and I think at some point, we will need to do it at least for the CLI when things get more complicated.
About naming, I know that Julia just calls it "project", but I would rather call it I think that at least the word "path" needs to be in the name. |
And one more comment: can we use |
I used |
@fonsp I renamed the field to I also changed the default to for the env variable comment, I think I'll just open a separate PR to polish this part directly later, given it's not quite related to this one. |
Is this good to merge now? @fonsp |
I'm just working on something else and then I'll merge! |
thanks! just let me know if there is anything else need me to revise. |
I made some changes - what do you think? |
yeah looks good. I should have used a more compatible way to write it. I'll move on to the other PR after this gets merged. @fonsp |
Thanks again! Sorry if I edited the PR too much - I deleted some configuration options with the idea that we want to keep flexibility for #366 |
Yeah, I'm good with your edits @fonsp I think this PR only aims to implement the feature first, so there is something you can keep track of. I'm polishing the environment variable handling and trying to allow control more compiler flags to the workspace process in #367 and make the environment variables of Pluto more consistent. I'll merge your edits to that PR later, after this gets merged. |
I haven't actually tried this PR. I guess the checks are:
|
I don't know how to test this either... I guess to test this either we need mock workspace function or somehow execute the test in a workspace. I tried this PR tho, my notebook at least shows the correct project environment. |
Which of the three did you test? |
@fonsp I can check things above manually, but I'm not sure how to write them as tests. if you start if you start with if we open a notebook directly under some project using Pluto
s = Pluto.ServerSession()
nb = Pluto.SessionActions.open(s, "sample/Basic.jl"; project="sample")
Pluto.run(;session=s) The notebook will now start at the specified project directory ("sample/Project.toml" in this case) |
Perfect, thanks for checking! |
I have |
This reverts commit d57e1f7.
The behaviour that |
Ah, thanks. Two minor comments, feel free to consider or not:
|
@marius311 note when you run Conceptually, the notebook process is not the same with server process, thus it should not inherit environment from server process or share the same environment variable. So it should be distinguished from I think for
The default will be
if you are using REPL, eventually there will only be one way to config Pluto via the |
This PR implements per notebook env in the backend. I think no matter what you want to do with the frontend (#142 ), this always needs to be handled in the backend.
What this PR does is basically attach a new field
project
to everyNotebook
object. It's an empty string by default, thus the default behavior is the same as current. But now one can set the process environment via Pluto's API:open
andrun
, e.gOr you can set the default global environment used in a Pluto server process via
JULIA_PLUTO_PROJECT
orPLUTO_PROJECT
environment variable, wherethe priority is set to the following:
project
kwargs in all Pluto APIsJULIA_PLUTO_PROJECT
PLUTO_PROJECT
I will implement the custom local environment feature in pluto CLI after this gets merged.I implemented the custom local environment feature in Pluto CLI: fonsp/PlutoUtils.jl#9 I think the Pluto CLI should be able to share a bunch of environment handling in Comonicon. I think this should resolve most of the problem in #142