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

[MNT] isolate optuna and statsmodels as soft dependencies #1629

Merged
merged 13 commits into from
Sep 4, 2024

Conversation

fkiraly
Copy link
Collaborator

@fkiraly fkiraly commented Aug 29, 2024

Isolates optuna, optuna-integration, and statsmodels as soft dependencies in a new soft dep set all_extras to collect all soft dependencies, and in another soft dep set tuning. Towards #1616

This was quite simple, since the imports happen in only one very narrow location, the optimize_hyperparameters utility which is also not core architecturally.

@fkiraly fkiraly added the dependencies Pull requests that update a dependency file label Aug 29, 2024
@codecov-commenter
Copy link

codecov-commenter commented Aug 29, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.19%. Comparing base (0a1e784) to head (5f50b20).

Files with missing lines Patch % Lines
...sting/models/temporal_fusion_transformer/tuning.py 91.66% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1629      +/-   ##
==========================================
- Coverage   90.20%   90.19%   -0.02%     
==========================================
  Files          32       32              
  Lines        4729     4734       +5     
==========================================
+ Hits         4266     4270       +4     
- Misses        463      464       +1     
Flag Coverage Δ
cpu 90.19% <91.66%> (-0.02%) ⬇️
pytest 90.19% <91.66%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@yarnabrina
Copy link
Member

If you do not mind, I'll request you to consider these changes in this PR. I have two points to request you to consider.

  1. In sktime, we quite often face issues as a lot of dependencies are part of public all_extras and their interdependcies and conflicts create weird situations in testing and debugging. I think it may be better to have a plan with everyone who are planning to help maintenance here, e.g. Benedikt, Xinyu, Felix etc.

  2. pytorch-forecasting is quite popular along with its tuning interface. If it suddenly looses those as part of main installation, I fear that may appear as a significant breaking change for a considerable part of its user base.

@fkiraly
Copy link
Collaborator Author

fkiraly commented Aug 30, 2024

Ok, point taken.

Some comments and options for discussion.

  • Comment: assuming we do agree (which we might not) that these deps are better as soft deps, the target situation of this PR, i.e., all soft deps in a single soft dep sets, is Pareto better than having them in the core dep set.
    • Of course I'm open to alternative suggestions, although there seem to be only a small number of soft deps.
  • Regarding "no breaking change:
    • I do not think pytorch-forecasting had this policy before.
    • This exact code locus already went through what probably was an annoying breaking change caused by optuna. Namely, the additional soft dep optuna-integration was suddelny required as optuna reached 3.3.0. Users who went through this already have explicit install of optuna-integration, which requires optuna, so their code will not break.
      • I think this is a strong argument that code will not break downstream, as downstream users who use optuna above 3.3.0 will have some code that installs optuna-integration, which installs optuna, which in turn prevents this code from breaking. Is this logically correct?
    • Alternatively: how about we do this on windows and pyhton>3.10? These versions were not officially supported before at the last release, so we can handle the dep sets as we wish, or worry about breaking changes under these conditions since there are no pre-existing users who use the package "officially" in these environments.
    • We could also adopt the sktime deprecation strategy where we announce, say, 3 months in advanec that certain packages will move to be a soft deps. As said, pytorch-forecasting users will be familiar with the situation (and likely watch for such messages) since optuna did a similar thing recently, see above.

@fkiraly
Copy link
Collaborator Author

fkiraly commented Aug 30, 2024

Pinging everyone @yarnabrina mentioned: @benHeid, @XinyuWuu, @fnhirwa

@fkiraly
Copy link
Collaborator Author

fkiraly commented Aug 30, 2024

On a more architectural level, and "defining scope", the tuner seems more like something that should be in optuna-integrations rather than in pytorch-forecasting proper. It's already apparent from the coupling which is very localized, and the general scope of content in pytorch-forecasting. This is perhaps not a change we want to make, it's just an architectural statement from the a-priori perspective of "all other things being equal" (e.g., not having to worry about breaking code, separate package management processes, testing, etc, which we obviously have to).

The closest thing to making it actually external is making it a soft dependency, which mitigates things like integration, release, testing.

Side note: my assessment is different for the sktime optuna tuner - that one is sktime API compliant, and sktime has its dependency management layer, these two arguments make it less clear where it should live, although it could also live in optuna-integrations and be indexed from sktime.

@fkiraly
Copy link
Collaborator Author

fkiraly commented Aug 30, 2024

On a more personal usage level, i.e., considering myself as an user and my opinions in that respect:

  • I don't want a models/algorithms package force-install a tuning package, e.g., optuna (unless of course it's a package primarily about tuning), since there are many similar packages and they all tend to have heavy-ish dependencies
  • I don't want a models/algorithms package force-install a plotting package, e.g., matplotlib, since it's unrelated scope, it's front-end, so I cannot run the business logic without front-end dependencies. It's also a relatively heavy import.

@fkiraly fkiraly added the maintenance Continuous integration, unit testing & package distribution label Aug 31, 2024
@fkiraly
Copy link
Collaborator Author

fkiraly commented Sep 4, 2024

from discussion on discord: agreed on a compromise solution where the tuning soft deps go into a tuning soft dependency set.

@fkiraly fkiraly merged commit 6061af6 into master Sep 4, 2024
37 checks passed
fkiraly pushed a commit that referenced this pull request Oct 3, 2024
Relaxes `numpy` bound to `numpy<3.0.0`.

(updated by @fkiraly - removed the `optuna` bound which has been dealt
with in #1629)

---

It should work with newer `numpy` and `optuna` version after #1599.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file maintenance Continuous integration, unit testing & package distribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants