⚙ new config system without environment variables #367

Roger-luo · 2020-09-05T22:27:19Z

This PR makes most of the path and env relocatable by runtime evaluating them (instead of compile time) and enable all Pluto environment variables to have a prefix JULIA_ or not. (note this needs #341 to be merged first)

Roger-luo · 2020-09-09T19:42:32Z

src/evaluation/WorkspaceManager.jl

@@ -82,7 +82,7 @@ function create_emptyworkspacemodule(pid::Integer)::Symbol
 end

 function default_environment_path(::Nothing)
-    get_pl_env("PLUTO_DEFAULT_ENVIRONMENT_PATH")


@fonsp sorry I didn't bring this up in last PR. I'm wondering what's the reason to change PLUTO_PROJECT to PLUTO_DEFAULT_ENVIRONMENT_PATH? I think at least at CLI side, this variable is exactly the same as JULIA_PROJECT: it can be used to interact with some environment manager tool (e.g direnv) to control the environment variable, but when pluto --project is used, it gets overwrite by --project. I find PLUTO_PROJECT is more consistent with Pkg, which is easier to remember, what do you think?

I think this name is short enough to be explicit. If you want to make the environment variable name actually matches the field name in the structs, I think renaming the internal field names is probably better.

Roger-luo · 2020-09-09T19:44:09Z

There is a few things I will update if @fonsp agrees with that:

allow all environment variables to have a JULIA_ prefix version. I think this can make things more explicit
trying to remove paths declared as const, this will prevent people from building distributable system image for Pluto

Roger-luo · 2020-09-09T19:52:41Z

src/evaluation/WorkspaceManager.jl

@@ -97,6 +97,9 @@ function resolved_environment_path(notebook::Notebook)
    else
        if isabspath(notebook.environment_path)
            tamepath(notebook.environment_path)
+        elseif notebook.environment_path == "@."


this fix the issue that when notebooks are specified with @. the project is using notebook_dir/@. path.

Roger-luo · 2020-09-09T21:25:19Z

there a few other things I just realize that the workspace process is currently spawned by inheriting all main process flags - which is something we don't want. e.g if I start the main process in CLI with compile=min, the notebook process will also start with that. I guess it's OK to start a Pluto server instance using compile=min given there is little to compile anyway, this can reduce the start-up time of Pluto itself too. But not OK for notebook workspace, since this will make runtime significantly slower on loops etc. On the other hand, I think the notebook workspace shouldn't use the default startup file unless specified, since things like OhMyREPL etc. are not for notebooks and can result in a long notebook start-up time, so we probably want to have --startup-file=no too, unless it is specified by user explicitly.

I'm thinking about implementing a kwargs interface in run and open instead of this PLUTO_PROJECT environment variable, and let Pluto CLI handle these compiler option related environment variables, so we don't end up in too much environment variables here. What do you think? @fonsp

fonsp · 2020-09-09T21:27:48Z

Thanks! I'll write a comment here: #366

Roger-luo · 2020-09-10T07:34:42Z

@fonsp I haven't make this branch runs, but maybe you want to have a look at the implementation I come up with for #366

Roger-luo · 2020-09-10T07:37:58Z

to be short, I basically make all the configs go into a Session and share them across the internals. Now workspace actually takes a set of full compiler options instead of just project, so we can make it not inheriting the main process compiler flags.

For interface level, I haven't implement the kwargs interface, but user only needs to learn what is a Session and how to config it now.

All paths are handle dynamically, this will make system image build happy.

Roger-luo · 2020-09-13T20:12:09Z

src/webserver/Session.jl

+    working_directory::String = default_working_directory()
+    run_notebook_on_load::Bool = true
+    workspace_use_distributed::Bool = true
+    require_token_for_open_links::Bool = true


put security struct back.

fonsp · 2020-09-14T01:59:14Z

@Roger-luo what do you think?

Roger-luo · 2020-09-14T04:20:30Z

perfect, now let me add:

the kwargs interface for run and open
add some tests as much as I can

then I think we are good to go!

Roger-luo · 2020-09-14T06:54:00Z

I just added the kwargs interface for run @fonsp does this looks good to you?

but somehow I cannot start a notebook session from this PR, something is broken... I need to check what's wrong. It seems that I need to take some more time to figure out this.

fonsp · 2020-09-14T08:58:32Z

~~What do you mean? Do you get an error?~~ Is it still broken?

~~Can you add a deprecation warning to the compat versions of Pluto.run?~~

fonsp · 2020-09-14T19:19:58Z

Let's merge?

…to pr/Roger-luo/367

Roger-luo · 2020-09-14T19:35:47Z

cool, it works now! I didn't realize there were other things that need to update. I tried to add some tests. I think it should be good to merge.

fonsp · 2020-09-14T19:36:15Z

Can you check why the tests failed?

Roger-luo · 2020-09-14T19:38:29Z

oops I wrote a wrong path in the test, let's see if it passes now.

Roger-luo · 2020-09-14T19:51:37Z

~~@fonsp tests all passed now, guess it's good to merge.~~ actually I think I need to add a few more tests, I just find the newly created notebook is not using @. by default.

Roger-luo · 2020-09-14T20:08:23Z

OK I think this is because when we assume each project starts with @.: currently if we specify the project at Pluto.run(;project="@."), this config will directly passed to the julia compiler, to let it decide what exactly @. means.

However, the newly created notebooks are in a tempdir which does not have Project.toml, thus it fallbacks to the global, even the user probably wants the Project.toml in current directory and specify it as @. (if the user specify it with a path, it's fine). I think this is more an interface related issue, we might need to expand @. ourselves before we pass it to addprocs? or should we keep the current behaviour? @fonsp

Roger-luo · 2020-09-14T20:24:26Z

umm, it seems before create_workspace is called, the pwd() is changed to the notebook folder already. Is this intended?

fonsp · 2020-09-14T20:40:19Z

About "@.": you decide, I am too confused, and this will be different by 0.13 anyways. Until then, anything is fine as long as most people launch their notebook into the global env by default.

About pwd: I don't know what you mean, but the pwd() according to your notebook's code should always equal the place where the notebook is stored. Changing the path should cd.

Roger-luo · 2020-09-14T20:57:49Z

I think I find out why, this is also the reason for #342 , basically, this is because Pluto will try to create a workspace in the server process to make UI loading faster. But it does not cd back when it finishes the job. This is causing

pwd is modified at server process, causing do not change pwd after exiting Pluto #342
@. is expanded in the wrong path, since pwd is not the actual pwd of the server process anymore

Roger-luo · 2020-09-14T20:59:01Z

src/evaluation/Run.jl

-		offline_workspace = WorkspaceManager.make_workspace(notebook, false)
-
+		# "A Workspace on the main process, used to prerender markdown before starting a notebook process for speedy UI."
+		offline_workspace = WorkspaceManager.make_workspace(


this workspace is created in the server process, which is causing the following workspace's compiler options pwd become incorrect.

fonsp · 2020-09-14T22:02:21Z

Aha! good catch

Roger-luo · 2020-09-14T22:21:58Z

I fixed #342 with a workaround for now in this PR, but I think it's not fully resolved:

if a user config the server process to not use Distributed but always run in the server process, the current behaviour will effect the pwd between different workspace inside the server process. I think in principal what is preferred is to execute each cell using cd(f, path) when it's executing in the server process.

now, all the notebook environment behaviour, no matter it's new notebook or not should be the same. to summarize the current behaviour:

Session Environment

Pluto.run set the environment to @. by default, which means:

if julia finds there is a Project.toml in parent directory, it will activate it
if julia finds no local Project.toml, it activates the global default environment (fallback to the current behaviour)

Pluto.run set by the user using relative path:

it will use joinpath(pwd(), relative_path), which is the relative path to current pwd

Pluto.run set by the user using absolute path:
- it will use the absolute path

Notebook Environment

one can create a notebook with notebook environment, or open a single notebook via SessionActions.open with compiler_options that the project kwargs is set to desired project path. We always use notebook environment if it's not nothing.

Roger-luo mentioned this pull request Sep 5, 2020

implement per notebook env in backend #341

Merged

Roger-luo closed this Sep 9, 2020

Roger-luo force-pushed the better_env branch from 3140108 to e29de19 Compare September 9, 2020 19:34

Roger-luo reopened this Sep 9, 2020

Roger-luo commented Sep 9, 2020

View reviewed changes

Roger-luo mentioned this pull request Sep 9, 2020

use @. as prior fonsp/PlutoUtils.jl#14

Closed

fonsp mentioned this pull request Sep 9, 2020

A better config system #366

Closed

use config structs

0302e50

Roger-luo force-pushed the better_env branch from bbaf1db to 0302e50 Compare September 10, 2020 07:33

rm old file

f4a3466

Roger-luo mentioned this pull request Sep 10, 2020

Pluto doesn't use Pkg environments? #393

Closed

Roger-luo changed the title ~~Better handling on env variables and path~~ new config system without environment variables Sep 10, 2020

Roger-luo commented Sep 13, 2020

View reviewed changes

fonsp added 3 commits September 14, 2020 03:46

use new config system everywhere

cb4c3a5

Merge branch 'master' into better_env

64cb6ba

compat fix

6291dcb

support kwargs interface

a3519a2

fonsp added 3 commits September 14, 2020 13:24

check kwarg validity

cf603fa

use new config system in frontend

eaae4b1

Pluto.run docs and deprecation warnings

eb06006

fonsp and others added 2 commits September 14, 2020 14:11

derp

49eea86

fix an arg error

d1cf01d

Roger-luo and others added 4 commits September 14, 2020 15:32

pretty printing error info

49905e8

add some tests for configs

dde665d

asdf

28a3b1c

Merge branch 'better_env' of https://github.com/roger-luo/pluto.jl in…

d4fbd27

…to pr/Roger-luo/367

fix test

a8ce862

fix test

6959c99

Roger-luo commented Sep 14, 2020

View reviewed changes

fix fonsp#342

1a3a53a

fonsp changed the title ~~new config system without environment variables~~ ⚙ new config system without environment variables Sep 14, 2020

fonsp merged commit d574dfb into fonsp:master Sep 14, 2020

Roger-luo mentioned this pull request Sep 14, 2020

do not change pwd after exiting Pluto #342

Closed

fonsp added this to the 🧭 v0.12 milestone Sep 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚙ new config system without environment variables #367

⚙ new config system without environment variables #367

Roger-luo commented Sep 5, 2020 •

edited

Roger-luo Sep 9, 2020

Roger-luo Sep 9, 2020

Roger-luo commented Sep 9, 2020

Roger-luo Sep 9, 2020

Roger-luo commented Sep 9, 2020 •

edited

fonsp commented Sep 9, 2020

Roger-luo commented Sep 10, 2020 •

edited

Roger-luo commented Sep 10, 2020 •

edited

Roger-luo Sep 13, 2020

fonsp commented Sep 14, 2020

Roger-luo commented Sep 14, 2020 •

edited

Roger-luo commented Sep 14, 2020 •

edited

fonsp commented Sep 14, 2020 •

edited

fonsp commented Sep 14, 2020

Roger-luo commented Sep 14, 2020

fonsp commented Sep 14, 2020

Roger-luo commented Sep 14, 2020

Roger-luo commented Sep 14, 2020 •

edited

Roger-luo commented Sep 14, 2020 •

edited

Roger-luo commented Sep 14, 2020

fonsp commented Sep 14, 2020

Roger-luo commented Sep 14, 2020

Roger-luo Sep 14, 2020

fonsp commented Sep 14, 2020

Roger-luo commented Sep 14, 2020

⚙ new config system without environment variables #367

⚙ new config system without environment variables #367

Conversation

Roger-luo commented Sep 5, 2020 • edited

Roger-luo Sep 9, 2020

Choose a reason for hiding this comment

Roger-luo Sep 9, 2020

Choose a reason for hiding this comment

Roger-luo commented Sep 9, 2020

Roger-luo Sep 9, 2020

Choose a reason for hiding this comment

Roger-luo commented Sep 9, 2020 • edited

fonsp commented Sep 9, 2020

Roger-luo commented Sep 10, 2020 • edited

Roger-luo commented Sep 10, 2020 • edited

Roger-luo Sep 13, 2020

Choose a reason for hiding this comment

fonsp commented Sep 14, 2020

Roger-luo commented Sep 14, 2020 • edited

Roger-luo commented Sep 14, 2020 • edited

fonsp commented Sep 14, 2020 • edited

fonsp commented Sep 14, 2020

Roger-luo commented Sep 14, 2020

fonsp commented Sep 14, 2020

Roger-luo commented Sep 14, 2020

Roger-luo commented Sep 14, 2020 • edited

Roger-luo commented Sep 14, 2020 • edited

Roger-luo commented Sep 14, 2020

fonsp commented Sep 14, 2020

Roger-luo commented Sep 14, 2020

Roger-luo Sep 14, 2020

Choose a reason for hiding this comment

fonsp commented Sep 14, 2020

Roger-luo commented Sep 14, 2020

Session Environment

Notebook Environment

Roger-luo commented Sep 5, 2020 •

edited

Roger-luo commented Sep 9, 2020 •

edited

Roger-luo commented Sep 10, 2020 •

edited

Roger-luo commented Sep 10, 2020 •

edited

Roger-luo commented Sep 14, 2020 •

edited

Roger-luo commented Sep 14, 2020 •

edited

fonsp commented Sep 14, 2020 •

edited

Roger-luo commented Sep 14, 2020 •

edited

Roger-luo commented Sep 14, 2020 •

edited