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

[MRG] Fix import error when torch is not installed. #463

Merged
merged 9 commits into from
Jul 5, 2023

Conversation

eicca
Copy link
Contributor

@eicca eicca commented Jul 4, 2023

Init file in tslearn/backend imports torch backend, which imports torch. This causes an import error when torch is not installed. This change fixes this issue by removing extra import in tslearn/backend/__init__.py

Additionally, this change fixes failing tests when torch is not installed.

Fixes #462

I tried to run the tests locally, but some of them fail with and without pytorch being installed, so I can't say for sure if I fixed all test cases.

When this is fixed, I think some users will appreciate a new release, since the latest version is breaking for everybody who didn't install pytorch. To make things slightly worse, the latest version of pytorch is incompatible with the latest version of tensorflow: pytorch/pytorch#97580, therefore the temporary fix of installing pytorch won't work in some cases.

Init file in tslearn/backend imports torch backend, which imports torch.
This causes an import error when torch is not installed.
This change fixes this issue by removing extra import in
tslearn/backend/__init__.py

Additionally, this change fixes failing tests when torch is not installed.

Fixes tslearn-team#462
@rtavenar
Copy link
Member

rtavenar commented Jul 4, 2023

Thanks @eicca for this prompt PR!

@YannCabanes coiuld you have a look?

@rtavenar
Copy link
Member

rtavenar commented Jul 4, 2023

Also, if I may add, it could be a good idea to have a test specifically tailored for the no-pytorch context to make sure that we don't make this mistake again in the future, what do you think?

@eicca
Copy link
Contributor Author

eicca commented Jul 4, 2023

Small update: I tried to compare the failing tests with and without pytorch and it looks like the same tests are failing. So I'd assume that this PR does fix all pytorch dependent tests :)

@rtavenar
Copy link
Member

rtavenar commented Jul 4, 2023

Small update: I tried to compare the failing tests with and without pytorch and it looks like the same tests are failing. So I'd assume that this PR does fix all pytorch dependent tests :)

Your edits to test_metrics.py should ease the process, I guess what's missing is to have a new test job and make sure it does not have torch installed.

@YannCabanes
Copy link
Contributor

Thanks a lot for your quick PR @eicca!

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 54.54% and project coverage change: -0.09 ⚠️

Comparison is base (f718dd5) 93.06% compared to head (6277c8e) 92.98%.

❗ Current head 6277c8e differs from pull request most recent head 7193d5c. Consider uploading reports for the commit 7193d5c to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #463      +/-   ##
==========================================
- Coverage   93.06%   92.98%   -0.09%     
==========================================
  Files          67       67              
  Lines        5625     5613      -12     
==========================================
- Hits         5235     5219      -16     
- Misses        390      394       +4     
Impacted Files Coverage Δ
tslearn/backend/__init__.py 100.00% <ø> (ø)
tslearn/tests/test_estimators.py 86.13% <0.00%> (-1.75%) ⬇️
tslearn/tests/test_metrics.py 99.35% <75.00%> (-0.65%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rtavenar rtavenar merged commit 38cbe2c into tslearn-team:main Jul 5, 2023
@rtavenar
Copy link
Member

rtavenar commented Jul 5, 2023

Thanks a lot @eicca ! This is now merged in main, and we will generate a new release from this patch!

@eicca
Copy link
Contributor Author

eicca commented Jul 5, 2023

Thanks a lot @eicca ! This is now merged in main, and we will generate a new release from this patch!

Thanks a lot @rtavenar and @YannCabanes for taking care of this!

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.

0.6.0 fails to import when pytorch is not installed with ModuleNotFoundError: No module named 'torch'
4 participants