Load (and use) template config from JSON file. #326

mkcor · 2019-07-25T11:51:08Z

Hello,

At the moment, conf.json files are detected in order to find all paths (populating, for instance, the nbconvert_template_paths attribute). But they are not actually loaded, let alone used.

As an aside, the reason for a945020 is pylint and https://github.com/ipython/traitlets/blob/7c6067ab10176d8cfd9039c4b96a04b725c31df6/traitlets/config/application.py#L773-L775, i.e., CLI config priority is already ensured within method load_config_file().

With d17af8a, we load template-related conf.json files and merge them into the voila config (not the other way around, thus ensuring CLI config priority). Morally, I wanted to do parallel
https://github.com/QuantStack/voila/blob/08ea126952b8ce26c6903757ffcef7696d7f7a79/voila/app.py#L327
and do self.load_config_file('conf', path=self.nbconvert_template_paths) but I ran into the issue of voila.configuration.VoilaConfiguration's methods not being recursive (for updating dict-like objects).

I have thus fallen back on using more generic traitlets.config.loader.Config objects (hence 4a5027c).

Example uses:

$ voila reveal.ipynb --template=reveal
$ voila reveal.ipynb --template=reveal --VoilaConfiguration.resources="{'reveal': {'transition': 'zoom'}}"
$ voila reveal.ipynb --template=reveal --VoilaConfiguration.resources="{'reveal': {'theme': 'simple', 'transition': 'zoom', 'scroll': True}}"

with upcoming https://github.com/QuantStack/voila-reveal :)

/cc @maartenbreddels

…r.Config

mkcor · 2019-07-25T13:45:21Z

I should have run the test suite locally before pushing!

mkcor · 2019-07-25T18:08:25Z

With this PR, I have taken up @maartenbreddels 's comment; I should also take up @SylvainCorlay 's responding comment.

maartenbreddels

Hi Marianne,

I don't think the code is working at the moment, it did not pick my conf.json of voila-vuetify's vuetify-default. Maybe it's good to add some resources to the test template https://github.com/QuantStack/voila/tree/master/tests/test_template and test if you pick this up.
Maybe we also should not 'just' read the conf.json file, but use a key of in the conf.json, e.g.:

# conf.json
{ "base_template": "bla",
  "traitlet_conf": { ...}
}

Any strong opinions on that?

cheers,

Maarten

maartenbreddels · 2019-07-26T08:53:27Z

voila/app.py

@@ -339,6 +338,16 @@ def setup_template_dirs(self):
                self.static_paths,
                self.template_paths,
                self.voila_configuration.template)
+            # then we load the template-related config
+            loader = JSONFileConfigLoader('conf.json', self.nbconvert_template_paths)


Currently the nbconvert_template_paths is not where conf.json is, it's in the directory above it.

I noticed https://github.com/mariobuikhuizen/voila-vuetify/blob/master/share/jupyter/voila/templates/vuetify-default/conf.json but followed in the footsteps of https://github.com/QuantStack/voila/blob/master/voila/paths.py (that you also, and rightfully so, pointed me to).

The current version of function collect_template_paths() searches for these directories
https://github.com/QuantStack/voila/blob/fa0dd3103acc8149a10f73a346ca2662dbb034f3/voila/paths.py#L31-L33
and assigns them to nbconvert_template_paths, static_paths, and tornado_template_paths respectively. So, I took it as hint about where conf.json files should be found:
https://github.com/QuantStack/voila/blob/08ea126952b8ce26c6903757ffcef7696d7f7a79/voila/paths.py#L55
and thought I was making the most of the nbconvert_template_paths attribute.

Did you guys make a conscious design decision? /cc @SylvainCorlay

maartenbreddels · 2019-07-26T08:56:23Z

voila/app.py

+                if os.path.exists(conf_file):
+                    conf = loader.load_config()
+                    # and update the overall config with it, preserving CLI config priority
+                    recursive_update(dict(conf), dict(self.voila_configuration.config))


Here you make a copy of conf, so conf will not be updated, I suggest you add tests for this.

I should include tests in this PR, you're right.

maartenbreddels · 2019-07-26T08:56:45Z

voila/app.py

+            # then we load the template-related config
+            loader = JSONFileConfigLoader('conf.json', self.nbconvert_template_paths)
+            for path in self.nbconvert_template_paths:
+                conf = {}


This line can be removed.

I think it's necessary if you have more than one template config file (say, PREFIX/share/jupyter/voila/templates/default/nbconvert_templates/conf.json and PREFIX/share/jupyter/voila/templates/reveal/nbconvert_templates/conf.json), but I'll double check. Thanks.

conf.json should not be in the nbconvert_templates directory btw, but the directory above that.
But, on line 347 you reassign to conf, with a different type, right?

Line 347, I'm loading the config (from conf.json) as a traitlets.config.loader.Config object; that's what I touched on in the PR description (part starting with "Morally, ...") and explained in yesterday's video call (wish you were there).

Unfortunately (wrt subsequent operations), unlike voila.configuration.VoilaConfiguration, traitlets.config.loader.Config doesn't inherit from traitlets.HasTraits. Among other benefits, the latter supports dotted attribute style access (and that's why I had to insert .config.VoilaConfiguration line 72).

maartenbreddels · 2019-07-26T08:57:25Z

voila/handler.py

@@ -69,7 +69,7 @@ def get(self, path=None):
        }

        # include potential extra resources
-        extra_resources = self.voila_configuration.resources
+        extra_resources = self.voila_configuration.config.VoilaConfiguration.resources.to_dict()


I don't understand why this is needed, could you explain?

Which addition? The .config.VoilaConfiguration part or the .to_dict()?

I'm noticing that it would be smarter to have the latter two lines afterwards, so the to_dict() method would be called only when entering the if block.

mkcor · 2019-08-28T10:26:19Z

Hi @maartenbreddels,

Maybe we also should not 'just' read the conf.json file, but use a key of in the conf.json, e.g.:
# conf.json
{ "base_template": "bla",
  "traitlet_conf": { ...}
}

A key of ...? I guess referring to the base template. That's a great question; I think it's healthier to check for suck a key, indeed. Thanks for bringing that up.

Also, since it's present in the (existing) vuetify template, I might as well follow this pattern.

maartenbreddels · 2019-08-28T10:58:33Z

Yes, I'd put it under "traitlet_configuration": {...}

mkcor · 2019-08-28T11:58:33Z

Yes, I'd put it under "traitlet_configuration": {...}

Not "voila_configuration": {...}? Not sure what you mean by 'it:' base template or resources?

mkcor · 2019-08-29T18:17:50Z

@maartenbreddels ok, I'm going for the following layout then:

$ cat conf.json
{
  "base_template": "reveal",
  "traitlet_configuration": {
    "resources": {
      "reveal": {
        "scroll": false,
        "theme": "simple",
        "transition": "zoom"
      }
    }
  }
}

mkcor · 2019-08-30T01:16:52Z

In the end, I had to go for

$ cat conf.json 
{
  "traitlet_configuration": {
    "base_template": "reveal",
    "resources": {
      "reveal": {
        "scroll": false,
        "theme": "simple",
        "transition": "zoom"
      }
    }
  }
}

otherwise I was running into a recursion error upon loading (via a traitlets Config method).

mkcor added 3 commits July 25, 2019 13:08

Access resources attribute via object of class traitlets.config.loade…

4a5027c

…r.Config

Load and include template config from JSON file

d17af8a

Improve code formally (minor)

a945020

mkcor added 2 commits July 25, 2019 16:24

Convert LazyConfigValue object to dict

2877c51

Check existence of config file before loading it

733211c

maartenbreddels requested changes Jul 26, 2019

View reviewed changes

mkcor mentioned this pull request Aug 15, 2019

Pass default values to resources in template. voila-dashboards/voila-reveal#1

Merged

mkcor mentioned this pull request Aug 30, 2019

Load template-related config from JSON file. #372

Merged

mkcor closed this Sep 10, 2019

mkcor deleted the load-template-config branch September 10, 2019 12:48

This was referenced Oct 29, 2019

voila-vuetify faills for voila 0.1.15 voila-dashboards/voila-vuetify#18

Closed

Standardize configuration file. voila-dashboards/voila-vuetify#19

Closed

Fixup voila configuration #454

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load (and use) template config from JSON file. #326

Load (and use) template config from JSON file. #326

mkcor commented Jul 25, 2019

mkcor commented Jul 25, 2019

mkcor commented Jul 25, 2019

maartenbreddels left a comment

maartenbreddels Jul 26, 2019

mkcor Jul 26, 2019

maartenbreddels Jul 26, 2019

mkcor Jul 26, 2019

maartenbreddels Jul 26, 2019

mkcor Jul 26, 2019

maartenbreddels Jul 26, 2019

mkcor Jul 26, 2019

maartenbreddels Jul 26, 2019

mkcor Jul 26, 2019

mkcor commented Aug 28, 2019

maartenbreddels commented Aug 28, 2019

mkcor commented Aug 28, 2019

mkcor commented Aug 29, 2019

mkcor commented Aug 30, 2019

Load (and use) template config from JSON file. #326

Load (and use) template config from JSON file. #326

Conversation

mkcor commented Jul 25, 2019

mkcor commented Jul 25, 2019

mkcor commented Jul 25, 2019

maartenbreddels left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkcor commented Aug 28, 2019

maartenbreddels commented Aug 28, 2019

mkcor commented Aug 28, 2019

mkcor commented Aug 29, 2019

mkcor commented Aug 30, 2019