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

Custom page config hook #1495

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

dfguerrerom
Copy link

@dfguerrerom dfguerrerom commented Oct 6, 2024

References

#1494

Code changes

I have added a new query param to set a custom labextensions_url, this url has to be used when is provided by the user.
I have created a draft of my own server to serve the custom lab extensions and it works as expected.

I'm not sure what is the best way to deal with the labextensions_path and how to use the ones that are served by the tornado server, so that' why I came doing a request to the server.

Any suggestion?


  • New configs in VoilaConfiguration:
    prelaunch_hook = Callable(
        default_value=None,
        allow_none=True,
        config=True,
        help="""A function that is called prior to the launch of a new kernel instance
            when a user visits the voila webpage. Used for custom user authorization
            or any other necessary pre-launch functions.

            Should be of the form:

            def hook(req: tornado.web.RequestHandler,
                    notebook: nbformat.NotebookNode,
                    cwd: str)

            Although most customizations can leverage templates, if you need access
            to the request object (e.g. to inspect cookies for authentication),
            or to modify the notebook itself (e.g. to inject some custom structure,
            although much of this can be done by interacting with the kernel
            in javascript) the prelaunch hook lets you do that.
            """,
    )

    page_config_hook = Callable(
        default_value=None,
        allow_none=True,
        config=True,
        help="""A function that is called to modify the page config for a given notebook.
            Should be of the form:

            def page_config_hook(
                current_page_config: Dict[str, Any],
                base_url: str,
                settings: Dict[str, Any],
                log: Logger,
                voila_configuration: VoilaConfiguration,
                notebook_path: str
            ) -> Dict[str, Any]:

            The hook receives the default page_config dictionary and all its parameters, it should
            return a dictionary that will be passed to the template as the `page_config` variable
            and the NotebookRenderer. This can be used to pass custom configuration.
            """,
    )
  • Reassign Voila.prelaunch_hook to VoilaConfiguration.prelaunch_hook, this will make prelaunch_hook work with the server extension.
  • Add deprecated warning for Voila.prelaunch_hook

User-facing changes

Users will have the opportunity to set their custom config_page settings

Backwards-incompatible changes

None

Copy link
Contributor

github-actions bot commented Oct 6, 2024

Binder 👈 Launch a Binder on branch dfguerrerom/voila/custom-labextension

@dfguerrerom dfguerrerom changed the title [WIP] set draft of customlabextension [WIP] custom labextensions url Oct 6, 2024
@trungleduc
Copy link
Member

I think a more generic solution is to allow users to modify the page_config object by a hook function. We have this mechanism for the prelaunch hook (https://voila.readthedocs.io/en/stable/customize.html#accessing-the-tornado-request-prelaunch-hook)

@dfguerrerom
Copy link
Author

@trungleduc what do you think about this implementation?

@dfguerrerom
Copy link
Author

while working on this, I have a question, why not use the kernel path to set the labextensions path?
If we can select which kernel to use within the kernelspec of the notebook, why not use also that path to get the labextensions?
With this hook I can handle the page config, but I don't know what notebook is actually requesting the page config... I would still need the query param or pass the notebook info to the hook?

@trungleduc
Copy link
Member

I'm thinking about a post-processing function that receives the current page_config dict and then returns a customized one. You can add more arguments to this hook function if needed.

@dfguerrerom
Copy link
Author

I'm thinking about a post-processing function that receives the current page_config dict and then returns a customized one. You can add more arguments to this hook function if needed.

I don't get the idea, what would be the difference?
Another question, did you also think about set hooks when the server_extension is launched?

What I'm doing to set the hooks is something like:

# voila.py 

from voila.app import Voila

def get_page_config_hook(....)

Voila.get_page_config_hook = get_page_config_hook

I'm mutating the Voila class and setting the hook, is that the way?

@trungleduc
Copy link
Member

I'm thinking about a post-processing function that receives the current page_config dict and then returns a customized one. You can add more arguments to this hook function if needed.

I don't get the idea, what would be the difference? Another question, did you also think about set hooks when the server_extension is launched?

It will be easier for someone who just needs a small change in the page_config. They don't need to redo all the lab extension gathering logic, etc...

What I'm doing to set the hooks is something like:

# voila.py 

from voila.app import Voila

def get_page_config_hook(....)

Voila.get_page_config_hook = get_page_config_hook

I'm mutating the Voila class and setting the hook, is that the way?

https://voila.readthedocs.io/en/stable/customize.html#adding-the-hook-function-to-voila

@dfguerrerom
Copy link
Author

I've created this post-processing function, does it look better to you?
Regarding the documentation for implementing the hook, I see a reference to an object "c" but I'm not sure where it comes from, is it the configuration?
In any case, if I go for the first approach which requires to modify the voila.py, the server_extension doesn't use the Voila object which is the one that holds the hook, am I right?

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

Regarding the documentation for implementing the hook, I see a reference to an object "c" but I'm not sure where it comes from, is it the configuration?

the c object will be injected by traitlets https://traitlets.readthedocs.io/en/stable/config.html#python-configuration-files

@trungleduc
Copy link
Member

In any case, if I go for the first approach which requires to modify the voila.py, the server_extension doesn't use the Voila object which is the one that holds the hook, am I right?

it's correct

@trungleduc
Copy link
Member

Please rebase this branch to get the CI fix from #1497

Copy link
Author

@dfguerrerom dfguerrerom left a comment

Choose a reason for hiding this comment

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

done!

@dfguerrerom
Copy link
Author

In any case, if I go for the first approach which requires to modify the voila.py, the server_extension doesn't use the Voila object which is the one that holds the hook, am I right?

it's correct

is there a reason for that? one would expect the hooks to work no matter which server is used, dont you think?

@trungleduc
Copy link
Member

In any case, if I go for the first approach which requires to modify the voila.py, the server_extension doesn't use the Voila object which is the one that holds the hook, am I right?

it's correct

is there a reason for that? one would expect the hooks to work no matter which server is used, dont you think?

could you elaborate more on your setup?

@dfguerrerom
Copy link
Author

dfguerrerom commented Oct 10, 2024

could you elaborate more on your setup?

We have a platform that allows users to run custom AWS instances, each of this instances start a jupyter server that we use to launch jupyter and render voila apps...
The apps are located in a shared folder that we mount into users instances alongside with their kernels... if they want to start an app, they use the jupyter server that is already running, and we just pass the path of the notebook... running another server just for voila dashboards would require more memory...

@trungleduc
Copy link
Member

In this case, you should use the configuration file (voila.json or voila.py), This file is picked up by both standalone app and the server extension

@dfguerrerom
Copy link
Author

In this case, you should use the configuration file (voila.json or voila.py), This file is picked up by both standalone app and the server extension

but the Voila class is never instantiated in server_extension.py so it doesn't know about the hooks and they are not passed to the handlers, and c is not automatically imported in that context...

@trungleduc
Copy link
Member

, and c is not automatically imported in that context...

yes, It's

Loads voila.json and voila.py config file from current working

@trungleduc trungleduc added the enhancement New feature or request label Oct 10, 2024
voila/app.py Outdated Show resolved Hide resolved
@trungleduc
Copy link
Member

Thanks! It looks neat now. Could you update the documentation about this feature? In the same section as the pre-launch hook.

@dfguerrerom
Copy link
Author

done! in 3f2ce38 I also added the page_config and prelaunch hooks to be used by the server extension , is that okay?

@dfguerrerom dfguerrerom marked this pull request as ready for review October 10, 2024 20:54
@trungleduc trungleduc self-requested a review October 10, 2024 22:10
@trungleduc
Copy link
Member

@dfguerrerom I moved the hooks to VoilaConfiguration to simplify the setup for the server extension case.
@martinRenou could you take a look at this PR?

@dfguerrerom dfguerrerom changed the title [WIP] custom labextensions url Custom page config hook Oct 11, 2024
@dfguerrerom
Copy link
Author

awesome @trungleduc ! Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants