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

fix: isolate required and optional dependencies #237

Merged
merged 9 commits into from
Oct 7, 2024
Merged

Conversation

slevang
Copy link
Contributor

@slevang slevang commented Oct 4, 2024

This makes the i/o dependencies, which are only required for the .save()/.load() methods, optional. I have an environment where I would really rather not have netcdf4 installed and realized xeofs was the reason it kept getting pulled in.

Along the same lines, numba is a heavyweight dependency and has very limited use here so far (only in GWPCA). numba is probably bigger in size than all other deps combined. statsmodels also has very minimal use (CPCCA). I'm tempted to try and make those optional as well, since probably the majority of people who use this package just stick to EOF. Wanted to get your thoughts first though @nicrie .

EDIT: this does make numba and statsmodels optional as well

@slevang slevang requested a review from nicrie October 4, 2024 03:13
Copy link
Contributor

@nicrie nicrie left a comment

Choose a reason for hiding this comment

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

LGTM! I also agree with your suggestion to make Numba and statsmodels optional. What should we call this additional category -- perhaps perf?

Could we add the different installation options to the installation instructions to clarify things for users? Specifically, users of MCA/CCA (which ultimately depend on CPCCA) should be aware that they need the statsmodels dependency.

@slevang slevang changed the title fix: make i/o dependencies optional fix: isolate required and optional dependencies Oct 4, 2024
@slevang slevang marked this pull request as ready for review October 4, 2024 20:21
@slevang
Copy link
Contributor Author

slevang commented Oct 4, 2024

Ok, I went ahead and moved numba and statsmodels into an optional group as well, with appropriate warnings in the install and contributing docs. I also parameterized the CI over minimal and complete dependencies to make sure the minimal version remains compatible with running the test suite, by skipping certain tests as needed.

I used etc as the miscellaneous group which is what xarray uses for sparse. Could also do accel (which xarray also uses, but this is mostly for things that speed up operations but still work without them) or perf. 🤷

@nicrie
Copy link
Contributor

nicrie commented Oct 7, 2024

hey @slevang I was quite packed over the last days, I will have a look now.

👍 for etc

Copy link
Contributor

@nicrie nicrie left a comment

Choose a reason for hiding this comment

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

shouldn't this be a class attribute?__

    self.extra_modules = []

sorry my bad somehow missed that you defined it before the __init__ method.

Copy link
Contributor

@nicrie nicrie left a comment

Choose a reason for hiding this comment

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

LGTM, if you want to address the one point regarding 5hnetcdf great! But even without from my side feel free to merge! :)

Copy link

codecov bot commented Oct 7, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 11 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@a6a32a0). Learn more about missing BASE report.

Files with missing lines Patch % Lines
xeofs/base_model.py 33.33% 8 Missing ⚠️
xeofs/cross/cpcca.py 50.00% 2 Missing ⚠️
xeofs/single/gwpca.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #237   +/-   ##
=======================================
  Coverage        ?   24.50%           
=======================================
  Files           ?       56           
  Lines           ?     3962           
  Branches        ?        0           
=======================================
  Hits            ?      971           
  Misses          ?     2991           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@slevang slevang merged commit a6c05e6 into main Oct 7, 2024
19 checks passed
@slevang slevang deleted the io-optional branch October 30, 2024 16:47
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.

2 participants