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

Include seaborn, plotly, and the MNE-BIDS-Pipeline #158

Merged
merged 34 commits into from
Dec 9, 2022

Conversation

mscheltienne
Copy link
Member

@mscheltienne mscheltienne commented Nov 29, 2022

Closes #157
Closes #156

Couple of points/questions:

  • pqdm is already listed (not that I know what it is)
  • Do you want to explicitly list matplotlib?
  • plotly is not distributed on conda-forge, do you want to add the plotly channel?

@mscheltienne mscheltienne marked this pull request as draft November 29, 2022 11:53
@larsoner
Copy link
Member

plotly is not distributed on conda-forge, do you want to add the plotly channel?

I see https://github.com/conda-forge/plotly-feedstock and locally:

$ conda create -n plotly -c conda-forge plotly
Collecting package metadata (current_repodata.json): done
Solving environment: done

... # a bunch of normal install stuff

I'll push a commit to add it here and we'll see if it works (it should).

The Windows error is not good, though 😒 It's probably not related to your changes, though, so we could merge once the others are green.

@mscheltienne
Copy link
Member Author

mscheltienne commented Nov 29, 2022

I've never used plotly, but the recommended install is via conda install -c plotly plotly=5.11.0 even if there is a feedstock on conda-forge. Any idea why?
Also the web page of the feedstock is a bit different from other packages, where the lines:

To install this package run one of the following:
conda install -c conda-forge ****
...

are missing. Any idea why?

That windows error had an interesting exit code 😞

@larsoner
Copy link
Member

but the recommended install is via conda install -c plotly plotly=5.11.0 even if there is a feedstock on conda-forge. Any idea why?

No I have never looked into this. It would be a good question for some support forum for them, or even https://github.com/conda-forge/plotly-feedstock/issues

@hoechenberger
Copy link
Member

I use plotly from conda-forge daily. This is in a Linux container running in Docker.

@hoechenberger
Copy link
Member

Maybe we can address #156 here too, it might help the solver...

@hoechenberger
Copy link
Member

Otherwise LGTM!

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
@hoechenberger hoechenberger marked this pull request as ready for review November 29, 2022 22:17
@hoechenberger hoechenberger enabled auto-merge (squash) November 29, 2022 22:17
@hoechenberger
Copy link
Member

Oh wait, we must bump the installer version number before merging!

@mscheltienne
Copy link
Member Author

Can you push the corresponding changes? I'm not sure I know where this version number is.

@hoechenberger
Copy link
Member

hoechenberger commented Nov 30, 2022

I somehow couldn't push to your branch, so I created a PR:
mscheltienne#1

@hoechenberger
Copy link
Member

@larsoner Windows installations are failing without any helpful information :(

@jaimergp
Copy link

jaimergp commented Dec 8, 2022

Maybe you are running into the NSIS 3.08 incompatibility issue? The hotfix would be to pin NSIS to 3.01.

OTOH, I am just done with some updates for napari so maybe you can use that now? The new packages are in napari/label/bundle_tools_2. There are some minor changes, but it should be essentially the same (only extra_files comes to mind).

@larsoner larsoner enabled auto-merge (squash) December 8, 2022 20:39
@mscheltienne
Copy link
Member Author

That's a meaningful error at least:

Traceback (most recent call last):
  File "C:\Users\runneradmin\mne-python\1.2.3_0\Scripts\mamba-script.py", line 6, in <module>
    from mamba.mamba import main
  File "C:\Users\runneradmin\mne-python\1.2.3_0\lib\site-packages\mamba\mamba.py", line 49, in <module>
    import libmambapy as api
  File "C:\Users\runneradmin\mne-python\1.2.3_0\lib\site-packages\libmambapy\__init__.py", line 7, in <module>
    raise e
  File "C:\Users\runneradmin\mne-python\1.2.3_0\lib\site-packages\libmambapy\__init__.py", line 4, in <module>
    from libmambapy.bindings import *  # noqa: F401,F403
ImportError: DLL load failed while importing bindings: The specified module could not be found.

@hoechenberger
Copy link
Member

Thanks for your suggestions, @jaimergp! Do you have any idea what might be the cause of the error message in the comment above (DLL import problem)? Could it be as simple as a missing runtime dependency?

@jaimergp
Copy link

jaimergp commented Dec 9, 2022

Maybe conda/conda#12161? There are some issues with DLLs on Windows in the last release, so keep an eye on the conda issue tracker.

@hoechenberger
Copy link
Member

Indeed, removing mamba from our dependency spec list fixed the problem (CI still fails because later steps expect mamba to be present)

@larsoner
Copy link
Member

larsoner commented Dec 9, 2022

@hoechenberger I'm going to try keeping mamba and just changing our mamba calls to conda in our tests. Then I can test the installer on Windows and see if mamba actually works there or not. That will tell us if end users should have it or not -- ideally we'd keep it and this will just be a CI/infrastructure hangup for us rather than an end-user usability bug

@hoechenberger
Copy link
Member

@larsoner Care to investigate? It might have to do with the way (in which shell) we run the installer on Windows

@larsoner
Copy link
Member

larsoner commented Dec 9, 2022

It might have to do with the way (in which shell) we run the installer on Windows

That's what I'm assuming/hoping, that way we can just work around it with conda ... here, knowing it works for end-users (assuming my test of mamba on the artifact actually works!). That way we can merge this and release a 1.2.3 installer, and look into our conda CI setup separately. It's always seemed a bit odd that we have to source the environment script every time just on Windows, but who knows 🤷

Since this sort of investigation can be a bit open-ended, I'd rather get this green, test it locally, and merge, rather than investigate more in this PR

@hoechenberger
Copy link
Member

But how would we actually get this one green? 🤔

@larsoner
Copy link
Member

larsoner commented Dec 9, 2022

It already is. It was failing because we used mamba in our commands, not (just) because it was installed. So I've already changed those commands to use conda instead.

@hoechenberger
Copy link
Member

Oh wow, I didn't realize! Ok cool!!!

@hoechenberger
Copy link
Member

@mscheltienne Do you dare to try the windows installer again?

@mscheltienne
Copy link
Member Author

Sure, but I won't have access to my Windows machine until Monday.. I'll test it then if this PR is still open :)

@larsoner
Copy link
Member

larsoner commented Dec 9, 2022

I just booted into windows, downloaded the installer, ran it, and verified that mne sys_info worked as well as mamba list from the MNE terminal, so in it goes. Good teamwork @hoechenberger @mscheltienne !

I'll cut a 1.2.3 installer release next. We're releasing 1.3 at the end of next week but it'll be good to make sure all the release mechanics are still working

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.

Include seaborn Pin ipywidgets to <8
4 participants