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

Add support for the selection of onnx, torch without requiring both installed #130

Merged
merged 16 commits into from Jun 26, 2023

Conversation

mscheltienne
Copy link
Member

It's still a draft.

IMO, we should ship one of the backend via pip install without requiring an optional key.
+1 to keep torch as "default" since that's what we started with. For now in the PR, I moved both dep. to an extra-key but I don't like this solution as it means that pip install mne_icalabel now leads to a "broken" package.
However, we don't need to have the same default on PyPI and conda-forge, or the same default on all platform if that can make packaging on conda-forge easier.

This PR moves the network part to a separate module. The selection itself is donoe in network/__init__.py in run_iclabel. Seems to work locally, but I haven't run the full tests.

Also, @adam2392 would you mind changing the black default to 88 characters to match MNE?

@mscheltienne mscheltienne marked this pull request as draft June 26, 2023 09:16
@mscheltienne
Copy link
Member Author

And for now in this PR, selection can not be done at the top level, mne_icalabel.label_components wil default to the first backend found in the order torch, onnx. Selection can be done at the next level, mne_icalabel.iclabel.iclabel_label_components with the backend argument. I was thinking of adding a set_backend similar to set_browser_backend which would modify the behavior of backend=None.

@mscheltienne mscheltienne changed the title Add support for the selection of the onnx, torch without requiring both installed Add support for the selection of onnx, torch without requiring both installed Jun 26, 2023
@adam2392
Copy link
Member

Also, @adam2392 would you mind changing the black default to 88 characters to match MNE?

Do you want me to add it here? If you run that, it will possibly change a lot of files. Nbd imo, but just wanted to make sure

@mscheltienne
Copy link
Member Author

Not in this PR, would make review too messy.

@mscheltienne
Copy link
Member Author

mscheltienne commented Jun 26, 2023

The mypy error seems to be a bug, c.f. python/mypy#6799
Looks like that change in XVFB fixed the linux CIs.

@mscheltienne
Copy link
Member Author

IMO, we should ship one of the backend via pip install without requiring an optional key.
+1 to keep torch as "default" since that's what we started with. For now in the PR, I moved both dep. to an extra-key but I don't like this solution as it means that pip install mne_icalabel now leads to a "broken" package.

@adam2392 What is your opinion on this?

@adam2392
Copy link
Member

IMO, we should ship one of the backend via pip install without requiring an optional key.
+1 to keep torch as "default" since that's what we started with. For now in the PR, I moved both dep. to an extra-key but I don't like this solution as it means that pip install mne_icalabel now leads to a "broken" package.

@adam2392 What is your opinion on this?

I like the idea of this in principle in the long run because ideally we can run some other stuff that doesn't require the deep-learning models. But for now, if we really do "require" torch, I would say let's default to that.

Copy link
Member

@adam2392 adam2392 left a comment

Choose a reason for hiding this comment

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

LGTM once CIs work

@mscheltienne
Copy link
Member Author

OK, great. Let's go with that then, and let's try to setup ONNX as the default in the conda-forge package if that helps packaging on windows. CIs should come back green, merging once that's the case.

@mscheltienne mscheltienne marked this pull request as ready for review June 26, 2023 14:35
@mscheltienne mscheltienne merged commit 814372b into mne-tools:main Jun 26, 2023
27 checks passed
@mscheltienne mscheltienne deleted the optional_dependencies branch June 26, 2023 14:35
mscheltienne added a commit to mscheltienne/mne-icalabel that referenced this pull request Jun 27, 2023
mscheltienne added a commit to mscheltienne/mne-icalabel that referenced this pull request Jun 27, 2023
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.

None yet

2 participants