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

Remove pin on ipywidgets #78

Open
liamhuber opened this issue Oct 12, 2022 · 10 comments
Open

Remove pin on ipywidgets #78

liamhuber opened this issue Oct 12, 2022 · 10 comments
Labels
wontfix This will not be worked on

Comments

@liamhuber
Copy link
Member

tldr; there is no reason we need the pin, and whenever nglview is compatible with more recent versions of ipywidgets we can remove it. Note: Version 8 of ipywidgets contains the upgrade to FontAwsome 5!

As far as I can tell, the root cause of #74 was not ipycanvas, but ipywidgets+nglview.

The errors like Failed to load model class 'CanvasModel' from module 'ipycanvas' in the Jupyter output and [Error] Plugin 'ipycanvas:plugin' failed to activate. in the JavaScript console may be caused by an issue with incompatible @jupyter-widgets/base versions and token conflicts. I discovered this by searching the follow-up JavaScript console error: [Error] Error: No provider for: jupyter.extensions.jupyterWidgetRegistry.. This understanding is supported by one of the JavaScript console warnings I get ([Warning] Unsatisfied version 6.0.1 of shared singleton module @jupyter-widgets/base (required ^2.0.0 || ^3.0.0 || ^4.0.0) (remoteEntry.249e02ea9427f8cca2ad.js, line 1)), even though I don't fully grok the issue.

The catch is, I seem to have been wrong about the conflict coming from ipycanvas+ipywidgets -- I think the canvas business was just a red herring that cropped up because multiple versions of @jupyter-widgets/base were getting loaded. The real issue seems to by nglview+ipywidgets, and in a cruel twist of fate the hand holding the dagger is pyiron's very own @pmrv 😂 He dipped his toes into nglview and frozen their ipywidgets dependency at v7!

The catch is that when both nglview and ipywidgets are explicitly included in the environment.yml for mybinder, mybinder seems to ignore this freeze and simply installs the latest version of ipywidgets. The catch is that I haven't been able to systematically reproduce the JavaScript error in a MWE, although I can reproduce another error and keeping ipywidgets down is letting ironflow's binder application work so far.

One day it would be nice to remove this pin (a) because there is no real reason for it inside ironflow, and (b) ipywidgets v8 contains the upgrade to FontAwesome5, which would let us resolve a bunch of TODOs for button icons.

I wanted some pretty detailed documentation of this:

  • For future me, because I will try hard to suppress this memory. Fighting dependency problems is miserable
  • So I don't forget to remove the pin and free up FontAwesome5 eventually
  • Because I had a really hard time finding google results to solve this, and the behaviour from mybinder (ignoring nglview's pin) surprised me, so I hope these terms will get picked up by google and help anyone else caught in the same trap
    • Failed to load model class 'CLASS' from module 'MODULE'
    • [Error] Error: No provider for: jupyter.extensions.jupyterWidgetRegistry.
    • [Warning] Unsatisfied version X.Y.Z of shared singleton module @jupyter-widgets/base (required ^X2.Y2.Z2 || ^X3.Y3.Z3)
@liamhuber liamhuber added the wontfix This will not be worked on label Oct 12, 2022
@pmrv
Copy link

pmrv commented Oct 12, 2022

I'm not sure I follow the full story here, but the reason for the pinning ipywidgets <=7 inside nglview is that it simply won't work with version 8. 7->8 is apparently a big transition in their APIs, and the nglview maintainer didn't make it sound likely that he would upgrade soon. It's a bit worrying to hear that conda disregards the pin when multiple packages specify a dependency.

@liamhuber
Copy link
Member Author

I'm not sure I follow the full story here, but the reason for the pinning ipywidgets <=7 inside nglview is that it simply won't work with version 8. 7->8 is apparently a big transition in their APIs, and the nglview maintainer didn't make it sound likely that he would upgrade soon.

Yes, I'm being quite tongue-in-cheek when I put the blame on you.

It's a bit worrying to hear that conda disregards the pin when multiple packages specify a dependency.

Yikes. Indeed, this is super scary! I was operating on the assumption that it wasn't conda, but rather something on top of conda that the binder folks were doing. Nope, conda itself is totally ignoring the pin in nglview! I made a new environment:

name: check_nglview_pin
channels:
- conda-forge
dependencies:
- python
- ipywidgets
- nglview

Then I get

conda list -n check_nglview_pin
...
ipywidgets                8.0.2              pyhd8ed1ab_1    conda-forge
...
nglview                   3.0.3              pyh8a188c0_0    conda-forge
...

Ok, nvm, I see the problem. nglview hasn't made a release since June 10, and your fix was merged on September 12. So the incompatibility is already there in v3.0.3, but we need v3.0.4 for conda to see the pin.

@liamhuber
Copy link
Member Author

liamhuber commented Oct 12, 2022

@pmrv I'll raise an issue over on nglview asking for a new release.

EDIT: Nevermind, I took a look at their issues page and there's no way that they are unaware a new release is badly needed.

@pmrv
Copy link

pmrv commented Oct 12, 2022

Ah ok, then we'll just wait for a release from them.

@niklassiemer
Copy link
Member

Actually, a plain new release would even not fix the issue. They 'just' need to update the dependencies in the conda-forge recipe to exclude the 8.x release there. The dependencies of the individual repositories are not checked for the dependency resolving on conda. This is one reason we allow for lower versions of h5io or numpy on there to allow for pyiron/tensorflow combinations in one environment... We should actually test our lower bounds as well...

@pmrv
Copy link

pmrv commented Oct 13, 2022

A relevant PR is already open on their feedstock, but seems to be stuck in limbo: conda-forge/nglview-feedstock#57

@liamhuber
Copy link
Member Author

Actually, a plain new release would even not fix the issue. They 'just' need to update the dependencies in the conda-forge recipe to exclude the 8.x release there. The dependencies of the individual repositories are not checked for the dependency resolving on conda. This is one reason we allow for lower versions of h5io or numpy on there to allow for pyiron/tensorflow combinations in one environment... We should actually test our lower bounds as well...

For my learning: if "a new release" includes updating the version on conda-feedstock to reflect the latest dependency limits, then a new release helps, right? It's just that a new github tag alone is insufficient?

@liamhuber
Copy link
Member Author

A relevant PR is already open on their feedstock, but seems to be stuck in limbo: conda-forge/nglview-feedstock#57

Yep, just discovered that myself immediately before reading this. It would be quite some learning for me to be able to fix it, and pinning the widgets dependency is a pretty easy band-aid, so I'm still content to wait for now.

@niklassiemer
Copy link
Member

Actually, a plain new release would even not fix the issue. They 'just' need to update the dependencies in the conda-forge recipe to exclude the 8.x release there. The dependencies of the individual repositories are not checked for the dependency resolving on conda. This is one reason we allow for lower versions of h5io or numpy on there to allow for pyiron/tensorflow combinations in one environment... We should actually test our lower bounds as well...

For my learning: if "a new release" includes updating the version on conda-feedstock to reflect the latest dependency limits, then a new release helps, right? It's just that a new github tag alone is insufficient?

Yes exactly! If we make a new release (i.e. tag) on pyiron_module the conda forge bot will make a new release PR there (and merge it if ci passes and it is configured to be merged - this is our setup). However that PR does not automatically change the dependencies of the conda package...

@jan-janssen
Copy link
Member

Yes exactly! If we make a new release (i.e. tag) on pyiron_module the conda forge bot will make a new release PR there (and merge it if ci passes and it is configured to be merged - this is our setup). However that PR does not automatically change the dependencies of the conda package...

There is some work in the conda-forge project to fix this - but it is not yet done. Hopefully we get it sooner rather than later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

4 participants