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

[minor] Dependency management with poetry #1202

Merged
merged 15 commits into from
Apr 15, 2023
Merged

[minor] Dependency management with poetry #1202

merged 15 commits into from
Apr 15, 2023

Conversation

noxan
Copy link
Collaborator

@noxan noxan commented Mar 7, 2023

🔬 Background

  • Poetry is a modern python dependency manager unifying all requirements and distribution related configuration

🔮 Key changes

❓ Discussion points

  1. Any reasons not to remove the requirements/*.txt setup?

📓 Open tasks

  1. Migrate the github actions to the new poetry setup (away from pip)

✨ Why poetry?

  1. It ensures package versions are compatible with the python version
  2. Ensures the right package dependency version and their dependencies are installed with its lock-file
  3. Automatically manages the virtual environment setup
  4. Integrates the publish workflow (pypi)
  5. Supports different dependency groups (e.g. tests, docs) and unifies them all into a single configuration file
  6. Easily keep track of all outdated packages (with poetry list --outdated) and update them accordingly

@noxan noxan marked this pull request as draft March 7, 2023 22:58
@github-actions
Copy link

github-actions bot commented Mar 7, 2023

Model Benchmark

Benchmark Metric main current diff
AirPassengers MAE_val 15.2698 15.2698 0.0%
AirPassengers RMSE_val 19.4209 19.4209 0.0%
AirPassengers Loss_val 0.00195 0.00195 0.0%
AirPassengers MAE 9.82902 9.82902 0.0%
AirPassengers RMSE 11.7005 11.7005 0.0%
AirPassengers Loss 0.00056 0.00056 0.0%
AirPassengers time 4.26428 4.51 5.76% ⚠️
YosemiteTemps MAE_val 1.71298 1.71298 0.0%
YosemiteTemps RMSE_val 2.2607 2.2607 0.0%
YosemiteTemps Loss_val 0.00095 0.00095 0.0%
YosemiteTemps MAE 1.45187 1.45187 0.0%
YosemiteTemps RMSE 2.16721 2.16721 0.0%
YosemiteTemps Loss 0.00066 0.00066 0.0%
YosemiteTemps time 100.316 104.55 4.22% ⚠️
PeytonManning MAE_val 0.58159 0.58159 0.0%
PeytonManning RMSE_val 0.72216 0.72216 0.0%
PeytonManning Loss_val 0.01239 0.01239 0.0%
PeytonManning MAE 0.41671 0.41671 0.0%
PeytonManning RMSE 0.55961 0.55961 0.0%
PeytonManning Loss 0.00612 0.00612 0.0%
PeytonManning time 11.8229 12.73 7.67%
Model training plots

Model Training

PeytonManning

YosemiteTemps

AirPassengers

@ourownstory
Copy link
Owner

@noxan I concur that this would be the right thing to do if our package would not be used within other packages or in environments with other packages.
However, tightening the requirements more than critically necessary will cause issues for libraries such as Darts using our package, as you can see from #1129
Also, users who want to use multiple different models in the same environment may run into issues.

Does poetry allow us to relax, rather than tighten the dependency bounds?

A small point is the familiarity most developers/users have with pip vs poetry.

@noxan
Copy link
Collaborator Author

noxan commented Mar 7, 2023

@ourownstory

However, tightening the requirements more than critically necessary will cause issues for libraries such as Darts using our package, as you can see from #1129

Also I agree with your argument of not defining them to tightly as it creates issues. But having unbound dependency versions creates other kinds of issues, e.g. our package breaks if another dependency introduces a breaking change. And in my experience it is better to be explicit about versions, it is easy to report a too tightly bound dependency and relax it. In contrary you can spend hours on figuring out which versions are incompatible, and why which package breaks. Especially if they are not explicitly defined, everything works fine, you recreate your development environment or a new developer joins the team and nothing works anymore - because the dependency versions luckily overlapped during your first project setup, just now things changed and nobody has a clue what...

Does poetry allow us to relax, rather than tighten the dependency bounds?

How tightly we define our dependencies is independent of poetry or pip. They both allow the same options.

Also, users who want to use multiple different models in the same environment may run into issues.

Into what kind of issues? That package versions are not compatible? At least it would be explicit then, good look trying to figure out which exact version of a dependency (3rd party) of your dependencies are incompatible to another.

A small point is the familiarity most developers/users have with pip vs poetry.

Agree - yet poetry also makes a lot of things a lot easier

@noxan
Copy link
Collaborator Author

noxan commented Mar 7, 2023

Another thing which I became aware of: we use a lot of dependencies which are actually not supporting our python version anymore, e.g. pandas dropped python 3.7 support with version 1.4. Our requirements allows anything pandas>=1.0.4 - apparently it still works but how knows for how long they break with python 3.7 - it is not even officially supported anymore.

Another example: pytorch is not compatible with python 3.11 yet - so we are neither.

@noxan noxan added the status: in development Pull requests which are in development label Mar 8, 2023
@noxan noxan force-pushed the poetry branch 2 times, most recently from 3172eda to 32f2f16 Compare April 14, 2023 01:27
@noxan noxan added status: needs review PR needs to be reviewed by Reviewer(s) and removed status: in development Pull requests which are in development labels Apr 14, 2023
@noxan noxan self-assigned this Apr 14, 2023
@noxan noxan marked this pull request as ready for review April 14, 2023 18:19
@noxan noxan added this to the Release 0.6.0 milestone Apr 14, 2023
Copy link
Collaborator

@LeonieFreisinger LeonieFreisinger left a comment

Choose a reason for hiding this comment

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

LGTM @noxan Please have a look at the failing test again

@codecov-commenter
Copy link

codecov-commenter commented Apr 15, 2023

Codecov Report

Merging #1202 (fa98462) into main (70ecb9e) will increase coverage by 0.10%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #1202      +/-   ##
==========================================
+ Coverage   89.67%   89.77%   +0.10%     
==========================================
  Files          38       38              
  Lines        4977     5055      +78     
==========================================
+ Hits         4463     4538      +75     
- Misses        514      517       +3     

see 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@noxan noxan changed the title [refactor] Dependency management with poetry [minor] Dependency management with poetry Apr 15, 2023
@noxan noxan added status: ready PR is ready to be merged and removed status: needs review PR needs to be reviewed by Reviewer(s) labels Apr 15, 2023
@noxan noxan merged commit f6de130 into main Apr 15, 2023
@noxan noxan deleted the poetry branch April 15, 2023 04:15
@noxan noxan modified the milestones: Release 0.6.0rc1, Release 0.6.0 Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready PR is ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants