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

Use extensionapp open_browser trait for serverapp config #217

Merged
merged 3 commits into from
May 6, 2020

Conversation

echarles
Copy link
Member

@echarles echarles commented May 1, 2020

This is a fix for #215

It uses the ExtensionApp open_browser trait for ServerApp config.

@echarles
Copy link
Member Author

echarles commented May 1, 2020

@Zsailer Do we have a replicable CI failure on ubuntu py3.7 (tests/test_terminal.py)?

@davidbrochart
Copy link
Contributor

I'm having the same failure over in #218.

@davidbrochart
Copy link
Contributor

I cannot reproduce the failure locally on an Ubuntu / python3.7.

@Zsailer
Copy link
Member

Zsailer commented May 1, 2020

Yeah, that test is flaky on all platforms. I've tried increasing the timeout in the await call, but still no luck.

Copy link
Member

@Zsailer Zsailer left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks, @echarles

@Zsailer Zsailer merged commit 15dd628 into jupyter-server:master May 6, 2020
@maartenbreddels
Copy link
Contributor

This PR breaks voila (with this branch voila-dashboards/voila#592) , cls.open_browser is the trait itself, not the value of the trait. It will lead to this exception:

traitlets.traitlets.TraitError: The 'open_browser' trait of a ServerApp instance must be a boolean, but a value of <traitlets.traitlets.Bool object at 0x10ed7f940> <class 'traitlets.traitlets.Bool'> was specified.

@echarles
Copy link
Member Author

Tried to get the value from the Bool trait without luck. Docs and google didn't help me. Any pointer?

@davidbrochart
Copy link
Contributor

I don't think it's possible. You can only observe traits and get the changes with old and new keys IIRC.

@blink1073
Copy link
Contributor

It is definitely possible. I'm afk but can give an example later

@maartenbreddels
Copy link
Contributor

You need the instance of cls to access the value, not the class. We can take the default value, but I guess we want the configured value right?

@echarles
Copy link
Member Author

Tried different methods (from class and instance), but I can only access the default value, not the configured value. We indeed need the configured value.

@Zsailer
Copy link
Member

Zsailer commented May 15, 2020

Wow—I completely missed this in the review. Sorry y'all.

I think this is the wrong way to do this—but I think this will be solved by idea proposed in #207—add a step for extensions to configure the server before the server is initialized.

@echarles
Copy link
Member Author

My bad... Before opening that PR I had done some sanity check launching a server and it was not breaking. The command I have used is jupyter server --ServerApp.jpserver_extensions="{'simple_ext1': True}" --SimpleExt1.open_browser=False which does not break and does not open the browser... but if you set --SimpleExt1.open_browser to True the browser is also not launched.

If you launch via entrypoint jupyter simple-ext1, you break...

I have been confused a few time with the difference in behavior when you launch via jupyter server or via the entrypoint...

Also not sure if the unit tests cover the entrypoints - Prolly was green because the test enable the extension via ServerApp.jpserver_extensions

@echarles
Copy link
Member Author

Agree as this is a global server config, we should have #207 to handle that.

@echarles
Copy link
Member Author

I have been confused a few time with the difference in behavior when you launch via jupyter server or via the entrypoint...

... so I have just opened #230

@echarles
Copy link
Member Author

@Zsailer @maartenbreddels Any hint to get the boolean from the Bool trait? If we can not get this working, I will open a new PR to rollback this breaking change.

@blink1073 blink1073 added this to the 1.0 Release milestone Sep 17, 2020
hMED22 pushed a commit to hMED22/jupyter_server that referenced this pull request Jan 23, 2023
…-browser

Use extensionapp open_browser trait for serverapp config
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.

6 participants