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

Reducing install_requires to minimum (& expand extras_require) and looser version ranges #56

Open
gmgeorg opened this issue Sep 4, 2023 · 3 comments

Comments

@gmgeorg
Copy link

gmgeorg commented Sep 4, 2023

At quick glance it seems that the current setup.py file is fully exhaustive on all dependencies as an absolute requirement including very specific ranges for versions. If at all possible, it would greatly improve compatibility w/ existing Python repos (and more people being able to use it w/o having to resolve conflicts) if the install_requires was only specifiying absolutely required modules (e.g., plotting or optuna is not really required to use this great package) and the minimum version date (>=) needed, instead of the (approximate) ~= range.

See also first accepted answer here:
https://stackoverflow.com/questions/6947988/when-to-use-pip-requirements-file-versus-install-requires-in-setup-py

Curious to learn more about whether this package has to be so specific/restrictive on the dependencies (e.g., suggest to use
https://stackoverflow.com/questions/10572603/specifying-optional-dependencies-in-pypi-python-setup-py

@StatMixedML
Copy link
Owner

Thanks for your suggestion.

As of now, model.py among others also imports optuna and the plotting packages. So all the requirements in the setup.py are required. I go thorugh your provided list of references. Thanks

@gmgeorg
Copy link
Author

gmgeorg commented Sep 6, 2023

Here is a better reference to suggest a pip install xgboostlss (minimum requirements) vs pip install xgboostlss[full] or sthg along those lines

https://stackoverflow.com/questions/6237946/optional-dependencies-in-distutils-pip

I looked through the code and yes it is currently the case that the implementation requires optuna, seaborn, matplotlib, pandas, shap (I think those are all the ones I found which are not critical for core functionality of XGBoostLSS.train() / .predict()). My suggestion was to decouple this in order to avoid the requirement. In particular, making plotting/tuning methods (!) of the core class unnecessarily ties them to the core functionality; if instead tuning and plotting would be function based that take an XGBoostLSS object as input, then you can decouple them.

This will also have the benefit that whenever you add nicer/better/more visualizations, users can just update the package and call the new functions on an existing object (loaded from disk for example), and don't have to re-initialize a new class, train / tune it again, just so they can use a new plotting funcationality.

Basically proposal would be sthg along the lines of

  • remove methods cv(self, ...), hyper_opt(self, ...) from XGBoostLSS object and make them functions cv(xgboostlss_object, ...) and hyper_opt(xgboostlss_object, ...) out of models.py into their own standalone tuning.py submodule. Then models.py does not require import optuna anymore; and if users don't want to use your tuning option, they are not required to import optuna anyway. tuning.py just wont work for them -- which is fine if they have other options.

  • move methods plot(self, ...), expectile_plot(self, ...) out of models.py into their own visualization.py module (again make them functions, not methods). Then in future if you add a new my_favorite_distribution_plot(xgboostlss_object, ...) function in that module, users can apply it to an existing (!) XGBoostLSS object w/o having to re-initialize/train/tune it again. Also here it would remove all the plotting (and even the pandas dependency AFAICT) from models.py.

This would accomplish the outcome that the core functionality in xgboostlss -- which AFAIU is training & predicting -- only depends on absolute minimum requirements; but any addon helper functions (like tuning or plotting) you provide are optional for installation/use. For handling the case when dependencies are not available you can then do sthg like here to let users know they need to install xgboostlss[full] to be able to access the tuning/plotting functionality.

@fkiraly
Copy link

fkiraly commented Jan 27, 2024

I would agree with you, @gmgeorg.

Wider version ranges ensure usability, whereas frequent bumps of lower bounds prevent compatibility with a wider range of systems.

Generally, I would strongly advise the same, separating out the tuning from the raw model - like in skpro where you have probabilistic tuners separate from the "atomic" regression models.

I would be happy to help with a little refactoring of this package, possibly copying some of the more general visualization and tuning logic to skpro? If @StatMixedML is ok with this, would you be up for working on this together, @gmgeorg?

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

No branches or pull requests

3 participants