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

Passing custom resources to voila configuration #301

Merged
merged 10 commits into from Jul 24, 2019

Conversation

mkcor
Copy link
Member

@mkcor mkcor commented Jul 12, 2019

Hello,

This PR overwrites #298 (previously submitted). Example use:

voila reveal.ipynb --template=reveal --reveal_transition=fade

The config additions are almost copied-pasted from https://github.com/jupyter/nbconvert/blob/76061f8df164a742fc039b2574508bd8cf7d96a1/nbconvert/exporters/slides.py#L105-L130 -- should I add a comment about it?

/cc @maartenbreddels

@SylvainCorlay
Copy link
Member

I like the idea of allowing to specify resources through the VoilaConfiguration object. An issue here is that this adds reveal-specific configurables to voila-core, which we cannot do for all possible templates.

Maybe a first way to do this would have the VoilaConfiguration have a resources Dict trait, which would not be reveal-specific. This may make setting options with the command line a bit difficult though.

@mkcor
Copy link
Member Author

mkcor commented Jul 14, 2019

@SylvainCorlay we are on the same wavelength... It's cumbersome to write JSON at the command line, but maybe great docs could make up for this inconvenience (I guess users will end up copying-pasting...).

Ok, so I'll implement the Dict trait.

@SylvainCorlay
Copy link
Member

SylvainCorlay commented Jul 14, 2019

I wonder if we could hack traitlets to have a Bunch syntax in the command line so that we can do

voila --foo.bar=baz

for any valid name bar. But I don't think that it would be in the scope of this PR to go beyond Dict.

@mkcor
Copy link
Member Author

mkcor commented Jul 14, 2019

Oh, I hadn't seen your comment about Bunch (which I hadn't heard of before). Please see 4144ed6.

I can see three additional improvements which would belong in this PR.

  • Modularization (abstraction): replace this if block by a function call, say resources.dict_merge(extra_resources) along the lines of these suggestions; is it a good idea to start a utils.py submodule in voila?
  • Validation: ideally we would want to check the validity of the values passed at the CLI and fall back on our defaults (when present) if a value isn't valid...
  • Instructions: I could add a "Coming soon!" subsection in the README to showcase that extra resources can be configured (even if the reveal template isn't published yet).

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

This is looking good to me. I left an inline comment about the sort of recursive dict.extend that you are doing.

Small note: I rebased the PR on master and add a commit removing the prepare reveal-specific stage. Also, extra_resources is directly called resources.

@SylvainCorlay SylvainCorlay changed the title Add reveal-required resources to voila configuration. Passing custom resources to voila configuration Jul 22, 2019
@SylvainCorlay
Copy link
Member

This looks good to me.

@maartenbreddels let me know if you have any comment on this before I merge. I would like to have a release including this so that @mkcor can iterate on the reveal template work.

voila/handler.py Outdated Show resolved Hide resolved
@maartenbreddels
Copy link
Member

LGTM, the failure was a timeout in nbconvert (we might want to increase that limit), restarted the travis job.

@maartenbreddels maartenbreddels self-requested a review July 24, 2019 07:40
@SylvainCorlay
Copy link
Member

OK, I pushed a commit removing the resources_reveal dictionary, which is specific to the reveal template.

(Since we can't elect any template-specific things to be part of the main voila package).

@maartenbreddels
Copy link
Member

Good point. I think if templates want to have specific resources, we could add that to conf.json, at least for defaults.

@SylvainCorlay
Copy link
Member

So the idea is that the reveal template should work without the resource dictionary providing theme, transition and scroll. In the voila template file, using reveal.get('theme', 'simple') instead of reveal.theme.

@SylvainCorlay SylvainCorlay merged commit 3cce6bf into voila-dashboards:master Jul 24, 2019
@mkcor mkcor deleted the config-reveal-resources branch July 24, 2019 09:40
@mkcor
Copy link
Member Author

mkcor commented Jul 24, 2019

Example use (update):

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

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