[WIP] Surface bookstore

[mpacer]

The purpose of this PR is to enable us to know on the front-end whether or not bookstore has been enabled.

This PR needs nteract/bookstore#44 in order to be able to report bookstoreEnabled: "True".

However, a lot of decisions and refactoring needed to happen to enable this.

Why did this require a refactor?

It made the most sense (IMO) to put the check as locally as possible to the handler itself, because that is where much of the rest of the information being sent to the "jupyter-config-data" script.

In order for us to do that, we needed to have access to bookstore settings. Bookstore settings are defined via the BookstoreSettings Configurable object and set via traitlet configuration assignment.

Previously, we had been instantiating a NteractConfig object inside the extension module using the nbapp's config. We were assigning values to that object (post instantiation) and were passing it through to our add_handlers function that was defined in the handlers module.

After some initial exploration, I discovered that we could not simply pass the nbapp's config object to the add_handlers function, because the config values were only being assigned to the local NteractConfig object and were not propagated back to the nbapp's config object. Values were also being assigned inside the add_handlers function (these also would not be propagated back to the nbapp's config object.

Unfortunately, simply passing the global config object through was insufficient, because for traitlet values that had not been directly assigned and were not explicitly configured, when accessed on the global config object they had no value and were still treated as instances of LazyConfigValue (see https://github.com/ipython/traitlets/blob/d07d70d81e82f655ac36841415721d5246876538/traitlets/config/loader.py#L65-L73)

Solution:
In order to get access to the global config object later, we add it to the webapp.settings and then explicitly instantiate the NteractConfig and the BookstoreSettings inside the actual handler code.

[mpacer]

@takluyver @Carreau @minrk @willingc do you have any insight why trying to access config.NteractConfig.asset_url (which has a default value) would return a LazyConfigValue rather than a realized value (if it hadn't been directly set in the code up to that point)?

[Carreau]

LazyConfigValue

That is so that it can allow things like c.append() in config file, until it actually encounter the traitlets.
LCV will store the append/prepend/update until the actual default config is encountered and then reify it.

[rgbkrk]

I'm a little bit confused about the necessity of this because we can tell if bookstore is enabled by looking at self.config.NotebookApp.nbserver_extensions["bookstore"]. This can be set in an init for NAppHandler like this:

    def initialize(self, *args, **kwargs):
        self.bookstore_enabled = self.config.NotebookApp.nbserver_extensions[
            "bookstore"
        ]

This does mean we'd want the bookstore module to be in charge of its own validation.

[mpacer]

self.config isn't the same object once we're inside the object iself. Also, we're currently using the initialize hook rather than overwriting the __init__ (which I think is actually the more idiomatic way to do it in tornado).

So what you're suggesting is that we don't actually do any validation in the nteract_on_jupyter extension, but that we only check to see if the serverextension is enabled?

This does mean we'd want the bookstore module to be in charge of its own validation.

I was thinking that it would be in charge of defining it's own validation but allow others to actually perform some of the validation. My thinking was that, if it is going to have "bookstore-aware" clients that are going to alter their UI on the basis of its functionality being available, it should provide a way for those clients to validate that the functionality is actually available in a manner that it defines. Adding the check that you're describing above seems like a great idea but I think it should supplement whatever we're providing in terms of bookstore self validation tooling.

[rgbkrk]

Whoops! You're so right and bookstore needs to switch to using initialize too for its handlers. 😬

[rgbkrk]

👍

[mpacer]

That is so that it can allow things like c.append() in config file, until it actually encounter the traitlets.
LCV will store the append/prepend/update until the actual default config is encountered and then reify it.

I understand that that is why it is there, why would it still not be reified when I attempted to directly access the value inside a handler (via config.NteractConfig.asset_url)?
@Carreau

[willingc]

The bookstore specific code looks fine to me. Reading through this code independent of M's changes that we have some code documentation work to do. There are too many app, nb, napp, config words that can have multiple meanings. I'm happy to go back through the Python here, document it, and see if I understand what is happening.

I'm a bit unclear as to what is generic extension handling vs bookstore extension handling and if the config changes breaks anything already deployed. Or in other words, what extensions exist beyond bookstore?

[willingc]

help='The application name. Default: 'nteract'. Configurable though most will keep the default')

[willingc]

Let's remove line 30

[minrk]

why would it still not be reified when I attempted to directly access the value inside a handler (via config.NteractConfig.asset_url)?

LazyConfigValue objects never get replaced with their resolved value on the config object because they can't know their value until they are loaded by the instance to be configured (and can resolve differently e.g. for different subclasses with different defaults).

Essentially, the values on Config objects should generally not be read directly, only the attributes on instances that they configure. The only reasons to read from Config object directly should be edge cases and deprecations.

So instead of:

url = config.NteractConfig.asset_url

you would always have somewhere:

nteract_config = NteractConfig(config=config) # or (parent=self)
...
asset_url = nteract_config.asset_url

Instantiating the class is the resolution step, which evaluates all the lazy values, dynamic defaults, etc. Additionally, it ensures that all values are defined, which is never true for the config itself.

[mpacer]

why would it still not be reified when I attempted to directly access the value inside a handler (via config.NteractConfig.asset_url)?

LazyConfigValue objects never get replaced with their resolved value on the config object because they can't know their value until they are loaded by the instance to be configured (and can resolve differently e.g. for different subclasses with different defaults).

Essentially, the values on Config objects should generally not be read directly, only the attributes on instances that they configure. The only reasons to read from Config object directly should be edge cases and deprecations.

So instead of:

url = config.NteractConfig.asset_url

you would always have somewhere:

nteract_config = NteractConfig(config=config) # or (parent=self)
...
asset_url = nteract_config.asset_url

Instantiating the class is the resolution step, which evaluates all the lazy values, dynamic defaults, etc. Additionally, it ensures that all values are defined, which is never true for the config itself.

That makes sense and is along the lines of what I was thinking, but I wanted to make sure that that was the pattern we should be aiming at.

Just to codify this:

1. When we want to set values dynamically inside the code itself (and override the defaults like we do in add_handlers), we can put them on the main config object. This allows us to pass around a single object through that is lazily evaluated.

2. If/when we need to access the values, we instantiate the Configurable class to make sure that everything has been appropriately resolved before access.

The open question is what to do in cases like
this:

nteract_config.version = (nteract_config.version or data['version'])

where we want to set a value conditionally based on the current value of the traitlet. Is the solution there to resolve it and then check the conditional? Are there likely to be any issues stemming from that?

[mpacer]

Also, already discussed this with @rgbkrk, but we won't be able to use

        self.bookstore_enabled = self.config.NotebookApp.nbserver_extensions[
            "bookstore"
        ]

due to a bug in the current notebook.config_manager.BaseJSONConfigManager and how it deals with the order of loading *_config.d/ config directories and instantiating serverextensions.

If you use bookstore's automatic enabling (as in its current released version) and try to access the config from inside the nteract_on_jupyter handler, the bookstore config (in …/etc/jupyter/jupyter_notebook_config.d/bookstore doesn't appear in that value.

E.g., when I have bookstore auto-enabled and print out the config available in the nteract_on_jupyter.extension.load_jupyter_server_extension code (with print(nbapp.config))
I get:

{'NotebookApp': {'nbserver_extensions': {'jupyter_nbextensions_configurator': True, 'nteract_on_jupyter': True}}…

and if I explicitly enable it I get

{'NotebookApp': {'nbserver_extensions': {'jupyter_nbextensions_configurator': True, 'nteract_on_jupyter': True, 'bookstore': True}}

I'll need to trace down where the bug is coming from before moving forward.

In the meantime, if we want this check (which seems like a good idea), another solution is to remove auto-enabling from the bookstore extension.

[minrk]

Just to codify this:

1. When we want to set values dynamically inside the code itself (and override the defaults like we do in add_handlers), we can put them on the main config object. This allows us to pass around a single object through that is lazily evaluated.
2. If/when we need to access the values, we instantiate the Configurable class to make sure that everything has been appropriately resolved before access.

Yup, that sounds right

The open question is what to do in cases like
this:

nteract_config.version = (nteract_config.version or data['version'])

where we want to set a value conditionally based on the current value of the traitlet. Is the solution there to resolve it and then check the conditional? Are there likely to be any issues stemming from that?

Yeah, that's sensible, or make it a dynamic default, e.g.

@default('version')
def _load_default_version(self):
    package_file = os.path.join(self.assets_dir, 'package.json')
     with open(package_file) as f:
         data = json.load(fid)
    return data['version']

which will work if you can be sure that self.assets_dir is correctly defined before .version is requested. But doing it the way you've defined is AOK as well.

Also, already discussed this with @rgbkrk, but we won't be able to use

        self.bookstore_enabled = self.config.NotebookApp.nbserver_extensions[
            "bookstore"
        ]

This is another case of interrogating the config object directly, which I would do my best to avoid. Ideally, there will be exactly zero references to self.config.ClassName in the code. Is there already a notebook app object? If so, interrogate its nbserver_extensions attribute instead of the config object. That should always be correct, while config will never be the only source of an attribute's value (accessing config directly ignores defaults, dynamic values, validation, etc.), so querying config directly will always give an incomplete picture of the situation. It should be very rare indeed that any code should be inspecting the config object directly instead of the object being configured.

[rgbkrk]

interrogate its nbserver_extensions attribute instead of the config object

We'd have to do this in the handler after all extensions are loaded I assume.

[minrk]

We'd have to do this in the handler after all extensions are loaded I assume.

Since all extension loading is done after finishing populating app.nbserver_extensions (which still needs jupyter/notebook#4376 to be correct), it should be okay to do this at extension loading time.

[minrk]

Here's a question, though: instead of asking the configuration or the parent application if yourself is enabled as an extension, maybe set a private flag when your extension is loaded that you can check? Then you aren't concerned with any state implementation details of the notebook server; you track yourself whether your extension is loaded:

_extension_loaded = False

def _load_jupyter_server_extension(self, app):
    global _extension_loaded
    _extension_loaded = True
    ...

[mpacer]

from talking with @stormpython we realized we should only surface this if it's validated and that bookstore's different pieces of functionality will require different validation

accordingly, this should become an object and not just a boolean.

[mpacer]

proposed new name "bookstoreValidated"

[mpacer]

closing in favour of #4241. We still should probably revisit our config at some point per @willingc's suggestion, but in the meantime the solution described there should get the job done.