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

auto enable nbextension when installing with pip #342

Merged
merged 1 commit into from
Mar 6, 2018

Conversation

maartenbreddels
Copy link
Contributor

This will enable rise for notebook >= 5.3. I do see a 404 for rise.css, i'm not sure that is ok.

@maartenbreddels
Copy link
Contributor Author

see jupyter/notebook#3116 for the changes that made this possible.

@damianavila
Copy link
Owner

Great, I was thinking about this the other day... some comments:

  1. We need to update the requirements here and here, because we need notebook >= 5.3.

  2. The postscripts step in the current conda recipe should be deleted, they are no longer necessary.

I do see a 404 for rise.css, I'm not sure that is ok.

That's OK. Not a problem.

@maartenbreddels
Copy link
Contributor Author

What most of the widgets do, is to keep the old method, and wait say a year until many people have moved. Another issue is that if the unlink script gets called, the extension is disabled, never to be abled again (it will override the defaults provided by this mechanism).

@damianavila
Copy link
Owner

What most of the widgets do, is to keep the old method, and wait say a year until many people have moved.

For those widgets, the new conda packages will be enabled by default when the build script is run (or the script cmd in the recipe), but then it is enabled again with the post-link? How do you solve that inconsistency?

Another issue is that if the unlink script gets called, the extension is disabled, never to be abled again (it will override the defaults provided by this mechanism).

I do not understand how the unlink script could give you problem if it is not there... can you explain me a little bit more? Thanks!!

@maartenbreddels
Copy link
Contributor Author

{prefix}/etc/jupyter/nbconfig/notebook.json contains what is enabled/disabled by jupyter nbextension .... The files in {prefix}/etc/jupyter/nbconfig/notebook.d/*.json contain what you can see as defaults, but the notebook.json overrides these, it it exists.
If the unlink script executes jupyter nbextension disable ..., it will leave in notebook.json the following:

{
  "load_extensions": {
    "ipyvolume/extension": false
  }
}

If after installing the new version (of say ipyvolume) which will add a file to the .d directory, it will not be enabled, since the settings of the json file takes precedence over the values from the .d directory.

I think the unlink script actually should not execute disable, since it does not leave an environment in the state it was in before the package was installed.
We should actually think how we're gonna solve this (cc @jasongrout).

In short, it doesn't hurt to merge this PR, it will make people using pip happy, but for the conda-forge users we cannot removed the link/unlink scripts yet.

@damianavila
Copy link
Owner

{prefix}/etc/jupyter/nbconfig/notebook.json contains what is enabled/disabled by jupyter nbextension .... The files in {prefix}/etc/jupyter/nbconfig/notebook.d/*.json contain what you can see as defaults, but the notebook.json overrides these, it it exists.

Oh, now it makes sense.

If after installing the new version (of say ipyvolume) which will add a file to the .d directory, it will not be enabled, since the settings of the json file takes precedence over the values from the .d directory.

Got it! Thanks for the additional explanation.

I think the unlink script actually should not execute disable, since it does not leave an environment in the state it was in before the package was installed.
We should actually think how we're gonna solve this (cc @jasongrout).

Interesting discussion...

In short, it doesn't hurt to merge this PR, it will make people using pip happy, but for the conda-forge users we cannot removed the link/unlink scripts yet.

Understood.

Still, this comment is pertinent, I think:

We need to update the requirements here and here, because we need notebook >= 5.3.

@parmentelat
Copy link
Collaborator

being a pip user myself, I would love this PR to make it in the next release

I can propose to handle the impact on the doc once the new one is on.

Also about the 404 that triggers concerning rise.css: this is sort of expected; the way we deal with ad hoc css (rise.css and thenotebookname.css) is to blindly try to load them into the browser, regardless of whether they exist or not; in other words I was not able to figure out a proper way to compute whether these files indeed are present or not - which would have been less sloppy because the 404 message of course can be alarming.

@damianavila
Copy link
Owner

being a pip user myself, I would love this PR to make it in the next release

It is the idea to include it.

I can propose to handle the impact on the doc once the new one is on.

Thanks.

Also about the 404

Not a big deal for now, I think.

@maartenbreddels
Copy link
Contributor Author

maartenbreddels commented Feb 22, 2018

We need to update the requirements here and here, because we need notebook >= 5.3.

We don't do this for ipywidget, ipyvolume, threejs or bqplot (AFAIK). We just mention that you still need to run this when you have < 5.3.
What do you think?

@damianavila
Copy link
Owner

What do you think?

That's ok, I guess... let's do the same than the other lib does.
Btw, I need to test this one before merging. Will do it soon.
Thanks!

@maartenbreddels
Copy link
Contributor Author

Thanks you for this great extension!

@damianavila
Copy link
Owner

I could finally test this one and looks good. Merging now and sorry for the delay!

@damianavila damianavila merged commit 3d8d5e3 into damianavila:master Mar 6, 2018
@damianavila damianavila added this to the 5.3.0 milestone Aug 15, 2018
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.

3 participants