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

A better config system #366

Closed
fonsp opened this issue Sep 5, 2020 · 16 comments
Closed

A better config system #366

fonsp opened this issue Sep 5, 2020 · 16 comments
Assignees
Labels
almost closed backend Concerning the julia server and runtime online deployment About deploying to binder, heroku, self-hosted

Comments

@fonsp
Copy link
Owner

fonsp commented Sep 5, 2020

@Roger-luo

Right now we have some ENV variables with a custom function to get default values - this was quickly put together for some flags in the automated testing, but I'd like to have something better.

First some use cases: (feel free to suggest more)

  • all the current switches to server behavior: https://github.com/fonsp/Pluto.jl/blob/v0.11.12/src/Pluto.jl#L21-L29
  • port, hostname and URL
  • security configuration (tokens, cors, etc)
  • launch with a couple of notebooks already running
  • for binder: launch with a predetermined set of notebooks in the main menu, and no further options
  • for binder: launch straight to the main menu
  • custom themes, custom keyboard shortcuts

Some requirements to make this easy:

  • We want more than just strings
  • We should use Julia code to set the config and nothing else.
  • I like that you can do
with_env("PLUTO_WORKSPACE_USE_DISTRIBUTED" => "false") do
    Pluto.run(1239)
end

Ideas:

  1. lots of keyword arguments to Pluto.run
  2. the ServerSession will contain all of this. A config file is just a .jl script that defines a ServerSession.
@fonsp fonsp added backend Concerning the julia server and runtime online deployment About deploying to binder, heroku, self-hosted labels Sep 5, 2020
@Roger-luo
Copy link
Contributor

I think we can consider use the same method I used in Comonicon's Comonicon.BuildTools.install for Pluto.run and Pluto.open (see also https://github.com/Roger-luo/Comonicon.jl/blob/master/src/tools/build.jl#L71)

  1. the entry function of Pluto forward a set kwargs to a config handling function, e.g
function Pluto.run(host, port;kwargs...)
  toml_config = read_toml(some_pluto_config_file)
  # merge config file configs and configs specified
  # by users using kwargs
  config = merge_config(toml_config, kwargs)
  # validate configs, see if there are anything incorrect
  validate_config(config)
  # actual run with all configs
  Pluto.run(configs) # This is dispatched on Pluto.run(::Dict)
end
  1. user can choose to specify things in kwargs, some env variables, or a config file, e.g Pluto.toml
  2. the priority takes in kwargs > Pluto.toml > env variables > default

As for config file, I actually suggest to just use a TOML, since you can limit what your config file can do in a TOML (given all we want to set up are PATHs, execution options etc. rather than arbitrary Julia code) but not an arbitrary Julia script. And note Pkg ships with a TOML and it will be a stdlib in 1.6.

@fonsp
Copy link
Owner Author

fonsp commented Sep 5, 2020

I think that I just want a way for a Julia script (like the CLI or the binder launch script) to run Pluto with extra configuration options - and not much more. So support for .toml & ENV should not be part of Pluto itself - if people need those then they should be able to write a tiny script to support it.

Maybe it's also a matter of taste - I prefer having only one option to do things, with more options only if necessary.

I think it should be something like

julia> Base.@kwdef mutable struct ServerConfiguration
           port::Integer=1234
           host::String="localhost"
           url::Union{String,Nothing}=nothing

           launch_browser::Bool=true

           security_mode::Pluto.SecurityMode=Something(1, 2, true)

           main_menu::Pluto.MainMenuMode=menu_default
       end

and you use it with

Pluto.run(; session=ServerSession(), configuration=ServerConfiguration())

@Roger-luo
Copy link
Contributor

I think this also depends on much configurations are there gonna be:

I'm personally OK with defining a Julia struct like the ServerConfig struct above if there are not much configurations, however, if there are a lot configurations let's say more than 10 configurations. It'd be much more readable if it's written in a TOML or some other separate config file.

As for environment variables, I think it's just necessary to tweak the default behaviour: imagine that a server admin wants to set the default environment to be specific folder, while allowing different server config to run on the same machine (e.g on different port and ip address with different security mode)

However, environment variables and config file can be set via the CLI indeed with a wrapper around Pluto.run. But I think then it's probably better to not set the config struct, but use kwargs, basically it looks like

function Pluto.run(;session, kwargs...)
   config = validate_config(kwargs) # this returns a Dict with default config set etc.
   run(session, config) # real run, based on input config
end

I think this will be more extensible than defining a struct, if the things configurable will keep growing, also it will make it more convenient for the CLI side to support environment variables and config file, since I can simply do

run(;session=ServerSession(), TOML.parsefile("Pluto.toml")...)

@fonsp
Copy link
Owner Author

fonsp commented Sep 6, 2020

Thanks! It sounds like kwargs to Pluto.run is the best option. I'll think about it some more

@fonsp
Copy link
Owner Author

fonsp commented Sep 9, 2020

I really like the idea of Pluto's configuration being contained in structs. You mentioned poor readability, but with Base.@kwdef they are very readable:

ServerConfiguration(
	port=123,
	launch_browser=false
) # other fields get the default value

I do not like environment variables inside Pluto's code, but I do see the need for them in the startup code for Pluto. So, kind if like we have now, it should be isolated into one file (like you did in #367), and from that point on, Pluto's code only deals with configuration structs.

And there should also be the options to bypass ENV variables and set up Pluto.run just by writing the configuration structs. If someone wants to run Pluto using a Julia process, then they shouldn't have to fall back to 'vintage' programming.

I added a third struct in my latest commit:
https://github.com/fonsp/Pluto.jl/blob/v0.11.14/src/webserver/WebServer.jl#L76

but I am not quite happy with the structure yet. I think that instead, ServerSession should contain all configuration, (i.e. ServerConfiguration and ServerSecurity), so that it can be more easily passed around the codebase. I'll try again soon.

@fonsp
Copy link
Owner Author

fonsp commented Sep 9, 2020

Also I quite like the division of Pluto.jl to have as few features/options as possible, and PlutoUtils.jl to have everything that a sysadmin would ever ask for. Something like .toml should definitely not go into Pluto.jl, but I would love to see it in PlutoUtils.

If possible, I would like the same with environment variables, but maybe that doesn't make sense

@Roger-luo
Copy link
Contributor

I agree environment variables is probably not very useful in terms of REPL-based interface in Pluto as long as we make it more configurable in Pluto.run. I just realize if all the configurations are passed by a struct or kwarg here, then we can just handle all the environment variables and config file on CLI side.

This makes more sense since only when one wants to use a CLI, will they need environment variables to setup some default behaviour.


Regarding to struct, I mean it still depends on how you do it, what I'm thinking about a config file looks like the following

name = "MyPlutoConfig"
show_banner="false"

[workspace]
distributed="true"
run_notebook_on_load="false"

[julia]
compile="all"
project="@."
sysimg="some path to a system image"
startup-file="no"
optimize=2

I think I'd expect this config file grows longer, and if we do it as kwargs, then we can just pass the parsed result to run directly, and split different options simply via platting ....

When someone find that we need some more options, we can simply parse those extra options by adding a pass, since like Project.toml, things that are not valid options are just ignored.

On the other hand, if we do it using struct, it cannot easily express the hierarchy of the config unless we actually do a few struct and compose them together, which is also a solution. It makes things more complicated, less flexible, but more secure.

@Roger-luo
Copy link
Contributor

Roger-luo commented Sep 9, 2020

To summarize,

struct is heavier, but safer. kwargs is lighter, but more flexible. But both can use toml on the PlutoUtils side.

On the other hand, users need to write less code if we support the kwargs interface, e.g

Pluto.run(;project="@.", compile="min", ...)

instead of

Pluto.run(;s=WorkspaceOptions(distributed=true), CompilerOptions(compile="all", ...))

tho, this is more a personal taste of interface, but I feel kwargs is at least simpler for users, since as a user I don't really need to know what is WorkspaceOptions or what is a Session etc.

Update:
Or we can consider do something similar to what Pkg does: support kwargs as interface, but internally use structs (which in Pkg's case are structs like PackageSpec etc.)

@fonsp
Copy link
Owner Author

fonsp commented Sep 9, 2020

Side note: what do you mean with the struct method being safer and more secure?

@fonsp
Copy link
Owner Author

fonsp commented Sep 9, 2020

Great that you brought up Pkg! Copying their method is a good idea - they probably put lots of thought into that design and the ergonomics of calling Pkg.something(...) is a similar problem to ours.

@fonsp
Copy link
Owner Author

fonsp commented Sep 9, 2020

So in that case, Pluto will have built in functionality that goes from a 'flat' structure of kwargs to the more structured combination of structs? And PlutoUtils.jl will be responsible for turning ENV variables and toml files and CLI arguments into a flat array of keyword arguments that are ...tted into Pluto.run?

@Roger-luo
Copy link
Contributor

Yes, I think we can just handle all the environment variables in PlutoUtils side as a way to config the CLI. And for Pluto.run there is only one way to configure it, is to use the kwargs interface of Pluto.run.

what do you mean with the struct method being safer and more secure?

directly passing the parsed dict to kwargs means it is still mutable in some sense, but if we use a struct, things that is passed to commands will be strictly what's inside those structs that has been defined explicitly.

@Roger-luo
Copy link
Contributor

Roger-luo commented Sep 9, 2020

There is only one potential issue to handle all the environment variables in PlutoUtils, is that it prevents someone to change the Pluto instance configuration using environment variable at runtime, since Pluto won't read up ENV anymore at runtime. But I feel this is likely shouldn't happen, please correct me if I'm wrong.

@fonsp
Copy link
Owner Author

fonsp commented Sep 9, 2020

True, let's just not support that then

@fonsp
Copy link
Owner Author

fonsp commented Sep 9, 2020

Okay nice! I can have a closer look at how Pkg works and have another go at making the structs nice during the coming days. But feel free to suggest a structure yourself

@Roger-luo
Copy link
Contributor

I think this is resolved by #367

@fonsp fonsp closed this as completed Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
almost closed backend Concerning the julia server and runtime online deployment About deploying to binder, heroku, self-hosted
Projects
None yet
Development

No branches or pull requests

2 participants