-
Notifications
You must be signed in to change notification settings - Fork 82
build: add pyproject.toml, poetry, and tox #1015
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
Conversation
5ab49bf to
b2503cf
Compare
There was a problem hiding this 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:
- Refactor of the way requirements are declared: from arrays in the
setup.pyto separate files in therequirementsfolder. - Pinning down all non-default requirements versions.
- Supporting constraining the
numpyversion. - Change the supported Python version from 3.6 and 3.7 to 3.7, 3.8 and 3.9.
- 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
numpyversions, 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?
|
Thanks for the review!
🙌
You're right, I forgot to add pyupgrade.
We could. I'm assuming 3.7 compatible code will also be 3.7+, albeit naively maybe.
Again, I'm naively assuming that if 1.17 works, and 1.21 works, all in between should work.
The idea is to:
It's definitely a trade-off. My assumption is that we don't need to:
Again it is just an assumption.
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. |
If that assumption held, then why wouldn't 3.6-compatible code be 3.7-compatible? 🤔
This sounds reasonable but I wouldn't advertise compatibility explicitly if there is no guaranteed testing.
Do we have documented cases of debugging / contribution being hindered and that would have been solved by pinning?
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? |
|
Thanks @MattiSG, I'll split this PR so to move incrementally while reaching consensus on the controversial bits 👍 |
b0f7f40 to
83c3884
Compare
|
Well I did a couple of changes, it looks better now :) |
cb12735 to
e83c43e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for your work!
This gets pretty close to shipping. I believe it might be appropriate to use this very impactful change to ship as many breaking deprecations as possible at once, as reusers are in for some adaptation work. I think of removing Dummy, for example. Do you see others?
5f2bd7e to
2452241
Compare
|
@MattiSG ? |
|
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 🙃 |
|
As things have converged toward the use of a |
Technical changes
pyproject.tomlto the repository.poetryto manage dependencies.toxto manage isolated tests.Deprecations
openfisca_core.commons.Dummy=>openfisca_core.commons.empty_cloneopenfisca_core.errors.ParameterNotFound=>openfisca_core.errors.ParameterNotFoundErroropenfisca_core.errors.VariableNameConflict=>openfisca_core.errors.VariableNameConflictErroropenfisca_core.errors.VariableNotFound=>openfisca_core.errors.VariableNotFoundErroropenfisca_core.formula_helpers.py=>openfisca_core.commonsopenfisca_core.memory_config.py=>openfisca_core.experimentalopenfisca_core.parameters.Bracket=>openfisca_core.errors.ParameterScaleBracketopenfisca_core.parameters.ParameterNotFound=>openfisca_core.errors.ParameterNotFoundErroropenfisca_core.parameters.ParameterParsingError=>openfisca_core.errors.ParameterParsingErroropenfisca_core.parameters.Scale=>openfisca_core.errors.ParameterScaleopenfisca_core.rates=>openfisca_core.commonsopenfisca_core.simulation_builder=>openfisca_core.simulationsopenfisca_core.simulations.CycleError=>openfisca_core.errors.CycleErroropenfisca_core.simulations.NaNCreationError=>openfisca_core.errors.NaNCreationErroropenfisca_core.simulations.SpiralError=>openfisca_core.errors.SpiralErroropenfisca_core.taxbenefitsystems.VariableNameConflict=>openfisca_core.errors.VariableNameConflictErroropenfisca_core.taxbenefitsystems.VariableNotFound=>openfisca_core.errors.VariableNotFoundErroropenfisca_core.taxscales.EmptyArgumentError=>openfisca_core.errors.EmptyArgumentError