search

openforcefield / openff-toolkit

The Open Forcefield Toolkit provides implementations of the SMIRNOFF format, parameterization engine, and other tools. Documentation available at http://open-forcefield-toolkit.readthedocs.io
http://openforcefield.org
MIT License
318 stars 92 forks source link

Toolkit Showcase examples has NGL errors #1718

Closed tdudgeon closed 1 year ago

tdudgeon commented 1 year ago

Describe the bug NGL viewer is not function in toolkit showcase notebook. Seems that this version of NGL is broken. Need to use an older one?

To Reproduce Clean checkout from github. Then:

mamba env create -f examples/environment.yaml 
conda activate offtk-examples
jupyter notebook

Then open the notebook and start executing the cells.

Output

Run the notebook cells. When running the first one that uses NGLView you get an error and nothing displays. The JS console shows:

image

Computing environment:

Additional context This page does mention steps needed to get NGL working, which have been necessary in the past, but the NGL version is 3.0.6 so this shouldn't be needed AND jupyter-nbextension is not present in the conda environment anyway.

mattwthompson commented 1 year ago

I get 3.0.6 pulled down in a fresh environment, which works for me in Jupyter Lab but not conventional notebooks (and seems to be what I was using in a separate environment yesterday without issue).

j-wags commented 1 year ago

I've made clean envs from mamba create -n temp openff-toolkit-examples which work fine, and made other installs using mamba env create -f examples/environment.yaml, which reproduce Tim's issue. The latter environment differs by pinning ipywidgets<8, resulting in different versions of ipywidgets (8.1.0 vs 7.8.0), jupyterlab_widgets (3.0.8 vs 1.1.5), and widgetsnbextension (4.0.8 vs 3.6.5).

I'm trying to remember the implications of changing examples/environment.yaml... If it's not used by anything else I'd recommend that we just scrap it and point users to mamba install openff-toolkit-examples to have fewer points of failure. But I need to make sure that nothing in the docs rendering or new centralized examples presentation isn't relying on it.

mattwthompson commented 1 year ago

I'd recommend that we just scrap

+1

My top-secret weapon (grep) doesn't find anything useful within the repo (CI uses separate files):

$ grep -r "examples/environment.yaml" .                              11:31:19  ☁  openmm-simulation ☂ ⚡
./.github/workflows/examples.yml:          # If openmmforcefields is included in examples/environment.yaml

I can't imagine something externally uses this file, and if that file isn't referenced anywhere it must not show up in relevant documentation.

tdudgeon commented 1 year ago

I re-tried launching using jupyter lab and I see what is essentially the same error.

j-wags commented 1 year ago

Hi @tdudgeon, thanks for the report. The issue is that we shouldn't have exposed that environment.yaml file at all because it's no longer up to date - Instead we should be directing users to either run mamba create -n openff-toolkit-examples openff-toolkit-examples or directing them to our more fully-fledged ecosystem examples page.

We're working on PR #1719 to remove the bad environment file and direct users somewhere more helpful, which will close this issue.

In the meantime, could you let me know whether creating a new env from mamba create -n openff-toolkit-examples openff-toolkit-examples resolves the issue for you?

tdudgeon commented 1 year ago

I would argue that having an environment.yaml file is indeed very useful as it makes the dependencies very transparent. It's one of the first things to look for to find out "OMG, do I have to install half the internet to run this". Shouldn't each example have its own environment.yaml file? A bit more work on your side, but if each one is tested independently ...

I will try using openff-toolkit-examples but won't be able to do that till next week. The ecosystem examples page page doesn't seem to have anything that describes how to set up each environment, though this does explain the basics.

j-wags commented 1 year ago

I would argue that having an environment.yaml file is indeed very useful as it makes the dependencies very transparent. It's one of the first things to look for to find out "OMG, do I have to install half the internet to run this". Shouldn't each example have its own environment.yaml file? A bit more work on your side, but if each one is tested independently ...

Yeah, I agree that having minimal envs for each example would be better, but we'd also need to maintain each one of them, which we can't afford at this time.

The ecosystem examples page page doesn't seem to have anything that describes how to set up each environment,

We don't communicate this well, but each example on the ecosystem examples page will pull its own environment yaml if users click the "download notebook" link. Right now this is a maximal env for most examples but we have the machinery to support per-example minimal envs if we end up with more time for maintenance on our hands.

Yoshanuikabundi commented 1 year ago

I'm trying to remember the implications of changing examples/environment.yaml... If it's not used by anything else I'd recommend that we just scrap it and point users to mamba install openff-toolkit-examples to have fewer points of failure.

I think it was a way to change example dependencies in CI before a new release, or it was used by Binder. Since we've moved away from Binder we shouldn't need it anymore. It might even be from before openff-toolkit-examples. I think at some point CI may have pointed to it and then been changed but I don't recall. +1 for removing it.

The ecosystem examples page page doesn't seem to have anything that describes how to set up each environment, though this does explain the basics.

The linked page does include a section describing this - it would be helpful to know how this could be made more clear!

tdudgeon commented 1 year ago

I've confirmed that NGL works fine if using a conda environment based on openff-toolkit-examples. Sorry for the delay in confirming this.