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

Load template-related config from JSON file. #372

Merged
merged 8 commits into from Oct 25, 2019

Conversation

mkcor
Copy link
Member

@mkcor mkcor commented Aug 30, 2019

Check this out, @maartenbreddels!

It's basically a cleaner version of #326 (which I will close). Still missing a test or two though...

I have used os.path.dirname to go one directory up.

Here, we are not simply reading whatever conf.json file we may find in a list of template paths, we are ensuring it corresponds to the template being used.

Demo style:

  • if no conf.json file lives in PREFIX/share/jupyter/voila/templates/reveal, then running
$ voila reveal.ipynb --template=reveal

will launch a slideshow where transitions slide (because 'slide' is the default, as set in the reveal template);

  • if this conf.json file lives in PREFIX/share/jupyter/voila/templates/reveal, then running
$ voila reveal.ipynb --template=reveal

will launch a slideshow where transitions zoom, because this config file is well-formed and says "transition": "zoom";

  • if this conf.json file lives in PREFIX/share/jupyter/voila/templates/reveal, then running
$ voila reveal.ipynb --template=reveal --VoilaConfiguration.resources="{'reveal': {'transition': 'fade'}}"

will launch a slideshow where transitions fade, because CLI config has preference; if conf.json specified a non-default resource (e.g., "scroll": true), it wouldn't be taken into account, unfortunately, because traitlets' Config merge is not recursive...

If we really wanted to merge all template-related configs recursively, always ensuring CLI priority, we would need to have the convoluted approach I had in #326.

The little contortions I had to do in handler.py come from covering all cases, e.g., for

$ voila reveal.ipynb --template=reveal --VoilaConfiguration.resources="{'reveal': {'transition': 'fade'}}"

to run regardless, with or without a config file somewhere. And all the config merging and passing around happening in app.py leads to the fact that: a) we need to get to self.voila_configuration.config.VoilaConfiguration.resources to find those extra resources (so painfully retrieved and combined) and b) self.voila_configuration.config.VoilaConfiguration.resources ends up being of type traitlets.config.loader.LazyConfigValue when neither a file nor CLI config passes extra resources (maybe I could prevent this by setting self.voila_configuration.config.VoilaConfiguration.resources = {} ahead of time in app.py).

@mkcor
Copy link
Member Author

mkcor commented Aug 30, 2019

@SylvainCorlay (following up on your earlier question, if I understand it better) with this hack, do you expect more than one way to pass resources at the CLI, for example something like

--VoilaConfiguration.resources="{'config': {'traitlet_configuration': {'resources': {'reveal': {'transition': 'fade'}}}}}"

in addition to

--VoilaConfiguration.resources="{'reveal': {'transition': 'fade'}}"

?

It's not trivial to find the alternative structure which would work, due to the convoluted config merge I'm doing here: https://github.com/QuantStack/voila/pull/372/files#diff-024301d95616622c19f49b8cd78d33c3R363-R367

@mkcor
Copy link
Member Author

mkcor commented Sep 5, 2019

@maartenbreddels could you please review, so I can wrap up this week? Thanks!

Copy link
Member

@maartenbreddels maartenbreddels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Marianne,

reviewing this I was wondering if we should use the JSONFileConfigLoader at all. Hope you find my review useful.

cheers,

Maarten

voila/app.py Outdated Show resolved Hide resolved
voila/app.py Outdated Show resolved Hide resolved
voila/app.py Outdated Show resolved Hide resolved
@SylvainCorlay
Copy link
Member

@mkcor the test failure is not related to your PR and was in master. It should be fixed it you rebase on the current master.

@SylvainCorlay
Copy link
Member

SylvainCorlay commented Oct 24, 2019

This looks good to me! Thanks for pushing this forward!

(I left a small comment inline).

@mkcor
Copy link
Member Author

mkcor commented Oct 25, 2019

@mkcor the test failure is not related to your PR and was in master. It should be fixed it you rebase on the current master.

Thank you! Done.

@SylvainCorlay
Copy link
Member

Thanks @mkcor !

@SylvainCorlay SylvainCorlay merged commit f62d716 into voila-dashboards:master Oct 25, 2019
@mkcor mkcor deleted the template-config-file branch October 25, 2019 12:25
@mkcor mkcor mentioned this pull request Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants