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

Improve dependency management #1015

Draft
wants to merge 37 commits into
base: master
Choose a base branch
from
Draft

Improve dependency management #1015

wants to merge 37 commits into from

Conversation

bonjourmauko
Copy link
Member

@bonjourmauko bonjourmauko commented May 1, 2021

Fixes #1012
Depends on #1168
Depends on openfisca/country-template#121

Technical changes

  • Extract requirements to separate files for easier contract enforcement.
  • Add explicit contract regarding supported dependencies.
  • Add constraint file to test against lower-bound NumPy.
  • Add extra dependencies.
    • Add coveralls (latest) to extra requirements.
    • Add twine (latest) to extra requirements.
    • Add wheel (latest) to extra requirements.
  • Pin non-distribution dependencies.
    • Pin autopep8 at latest.
    • Pin flake8 at latest.
    • Pin flake8-bugbear at latest.
    • Pin flake8-print at latest.
    • Pin pytest-cov at latest.
    • Pin mypy at latest.
    • Pin flask at 1.1.2.
    • Pin gunicorn at 20.1.0.
    • Pin flask-cors at 3.0.10.
    • Pin werkzeug at 1.0.1.
  • Relax distrubution dependencies.
    • Set dpath at >= 1.3.2, < 2.
    • Set psutil at >= 5.4.7, < 6.
    • Set sortedcontainers at >= 2, < 3.
  • Relax circular dependencies.
    • Relax openfisca-country-template.
    • Relax openfisca-extension-template.

Breaking changes

Expired deprecations

  • openfisca_core.commons.Dummy => openfisca_core.commons.empty_clone
  • openfisca_core.errors.ParameterNotFound => openfisca_core.errors.ParameterNotFoundError
  • openfisca_core.errors.VariableNameConflict => openfisca_core.errors.VariableNameConflictError
  • openfisca_core.errors.VariableNotFound => openfisca_core.errors.VariableNotFoundError
  • openfisca_core.formula_helpers.py => openfisca_core.commons
  • openfisca_core.memory_config.py => openfisca_core.experimental
  • openfisca_core.parameters.Bracket => openfisca_core.errors.ParameterScaleBracket
  • openfisca_core.parameters.ParameterNotFound => openfisca_core.errors.ParameterNotFoundError
  • openfisca_core.parameters.ParameterParsingError => openfisca_core.errors.ParameterParsingError
  • openfisca_core.parameters.Scale => openfisca_core.errors.ParameterScale
  • openfisca_core.rates => openfisca_core.commons
  • openfisca_core.simulation_builder => openfisca_core.simulations
  • openfisca_core.simulations.CycleError => openfisca_core.errors.CycleError
  • openfisca_core.simulations.NaNCreationError => openfisca_core.errors.NaNCreationError
  • openfisca_core.simulations.SpiralError => openfisca_core.errors.SpiralError
  • openfisca_core.taxbenefitsystems.VariableNameConflict => openfisca_core.errors.VariableNameConflictError
  • openfisca_core.taxbenefitsystems.VariableNotFound => openfisca_core.errors.VariableNotFoundError
  • openfisca_core.taxscales.EmptyArgumentError => openfisca_core.errors.EmptyArgumentError

@bonjourmauko bonjourmauko added the kind:build Pull requests that update a dependency file label May 1, 2021
@bonjourmauko bonjourmauko requested a review from a team May 1, 2021 21:10
@bonjourmauko bonjourmauko force-pushed the improve-deps branch 3 times, most recently from 5ab49bf to b2503cf Compare May 1, 2021 22:05
@bonjourmauko bonjourmauko requested a review from guillett May 6, 2021 20:12
Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

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

Exciting! 😃

I understand that this PR brings a few different sets of changes:

  1. Refactor of the way requirements are declared: from arrays in the setup.py to separate files in the requirements folder.
  2. Pinning down all non-default requirements versions.
  3. Supporting constraining the numpy version.
  4. Change the supported Python version from 3.6 and 3.7 to 3.7, 3.8 and 3.9.
  5. Refactor all CI-specific dependencies to the standard requirements declaration.

If this is correct, then:

  • I am very much in support of (5).
  • While in favour of it, I don't understand how this changeset enables (4). I believe we should add matrix testing in CI if we want to support multiple Python versions.
  • I don't understand the expected usage of (3): if we advertise support for multiple numpy versions, why do we test only against the oldest version?
  • I don't understand the added value of (2), while I see the cost of needing to deploy a new Core version whenever a patch is published in one of the dev requirements 😰
  • I am not sure I understand the added value of (1). I like how each set of requirements has its own file, but I also like that all requirements are in setup.py. What is the most standard in Python dependency management?

README.md Outdated Show resolved Hide resolved
requirements/debug Outdated Show resolved Hide resolved
requirements/debug Outdated Show resolved Hide resolved
requirements/dev Outdated Show resolved Hide resolved
requirements/dev Outdated Show resolved Hide resolved
requirements/tracker Outdated Show resolved Hide resolved
requirements/web-api Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@bonjourmauko
Copy link
Member Author

Thanks for the review!

  • I am very much in support of (5).

🙌

  • While in favour of it, I don't understand how this changeset enables (4).

You're right, I forgot to add pyupgrade.

I believe we should add matrix testing in CI if we want to support multiple Python versions.

We could. I'm assuming 3.7 compatible code will also be 3.7+, albeit naively maybe.

  • I don't understand the expected usage of (3): if we advertise support for multiple numpy versions, why do we test only against the oldest version?

Again, I'm naively assuming that if 1.17 works, and 1.21 works, all in between should work.

  • I don't understand the added value of (2)

The idea is to:

  1. reduce entropy for contributors —without impact for reusers
  2. being able to use pip without the legacy option —currently it is too slow as dep ranges are too broad

while I see the cost of needing to deploy a new Core version whenever a patch is published in one of the dev requirements 😰

It's definitely a trade-off.

My assumption is that we don't need to:

  • core dep ranges needs to be broad —we can't know reusers' constraints upfront
  • numpy specifically needs also to be retro-compatible, for the same reason
  • the rest do not, as dev is done locally, so we can live with outdated dev deps and update them once every 3 months for ex.

Again it is just an assumption.

  • I am not sure I understand the added value of (1). I like how each set of requirements has its own file, but I also like that all requirements are in setup.py. What is the most standard in Python dependency management?

Seems to be to use different files (both for requirements and for constraints), similar to what's proposed here.

I did actually borrowed the idea to both https://github.com/opendatateam/udata and https://github.com/jazzband/pip-tools.

@MattiSG
Copy link
Member

MattiSG commented Aug 24, 2021

I'm assuming 3.7 compatible code will also be 3.7+

If that assumption held, then why wouldn't 3.6-compatible code be 3.7-compatible? 🤔

I'm naively assuming that if 1.17 works, and 1.21 works

This sounds reasonable but I wouldn't advertise compatibility explicitly if there is no guaranteed testing.
Also, I'm not sure I understand how the system implemented in this PR checks for both upper and lower bounds. I understand it checks explicitly for the lower bound, and checks for some version matching version constraints, on which we have no control, and we assume it should be the latest but we are not certain (maybe the cache is old and the package manager is satisfied with it).

  1. reduce entropy for contributors —without impact for reusers

Do we have documented cases of debugging / contribution being hindered and that would have been solved by pinning?

we can't know reusers' constraints upfront

Exactly, that's why I'm a bit worried with pinning dependencies.

I'd love to have @openfisca/france-contrib-ipp, @guillett, @MaxGhenis… chime in on that. What would be the expected impact of pinning down dependencies for your teams?

@bonjourmauko
Copy link
Member Author

Thanks @MattiSG, I'll split this PR so to move incrementally while reaching consensus on the controversial bits 👍

@bonjourmauko
Copy link
Member Author

Well I did a couple of changes, it looks better now :)

@bonjourmauko bonjourmauko added the kind:theme A group of issues, directly tied to an OKR label Sep 29, 2021
@bonjourmauko bonjourmauko modified the milestone: Improve testing & releases Sep 29, 2021
@bonjourmauko bonjourmauko added the kind:roadmap A group of issues, constituting a delivery roadmap label Sep 29, 2021
@bonjourmauko
Copy link
Member Author

@MattiSG ?

@MattiSG
Copy link
Member

MattiSG commented Nov 11, 2021

When I see the impact on country-template (openfisca/country-template#121), I am clearly in favour of getting rid of the system of deprecation and actually using major version breaks. I fail to understand the added value of this delayed way of updating the API, and now we have PRs that are not backwards-compatible yet have no version bump 🙃

@bonjourmauko bonjourmauko marked this pull request as draft December 9, 2022 20:08
@bonjourmauko bonjourmauko removed kind:theme A group of issues, directly tied to an OKR kind:roadmap A group of issues, constituting a delivery roadmap labels Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:build Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants