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

Remove constraint on packages #1160

Merged
merged 6 commits into from
Dec 1, 2022

Conversation

bonjourmauko
Copy link
Member

@bonjourmauko bonjourmauko commented Nov 16, 2022

Fixes #1158

New Features

  • Lighter install by removing test packages from systematic install.

@bonjourmauko bonjourmauko requested review from MattiSG and a team November 16, 2022 20:23
@bonjourmauko bonjourmauko added the kind:build Pull requests that update a dependency file label Nov 16, 2022
CHANGELOG.md Outdated Show resolved Hide resolved
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.

Thanks @maukoquiroga for this exciting proposal!

  • It relies on the fact that the two templates do not have any dependency that would not be provided by the Core. At this stage (and for the foreseeable future), this is the case: they both only depend on Core. However, this should be documented at least in a comment.
  • The current README install procedure for development mode suggests pip install --editable .[dev] --use-deprecated=legacy-resolver. If I understand correctly, one would now need to use make install (which is consistent with Consolidate build workflow #1040), so this should be updated. A workaround would be to make sure that make test installs these test dependencies.
  • We lose the versioning of templates in testing, which means that builds are not reproducible: they might pass or fail one day and yield a different result with a different Template version. This is a regression, but I find it an acceptable cost considering that this is an edge case because of the slow change pace of Template.
  • Can you please confirm that this solves Simplify testing of new major versions #1158, and link it appropriately if that's the case? 🙂

openfisca_tasks/lint.mk Outdated Show resolved Hide resolved
openfisca_tasks/install.mk Outdated Show resolved Hide resolved
openfisca_tasks/install.mk Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
@bonjourmauko
Copy link
Member Author

Thanks @maukoquiroga for this exciting proposal!

* It relies on the fact that the two templates do not have any dependency that would not be provided by the Core. At this stage (and for the foreseeable future), this is the case: they both only depend on Core. However, this should be documented at least in a comment.

Correct. This can either be documented in a comment, or by creating a lock file in the packages (I'm doing that now).

* The current README install procedure for development mode suggests `pip install --editable .[dev] --use-deprecated=legacy-resolver`. If I understand correctly, one would now need to use `make install` (which is consistent with #1040), so this should be updated. A workaround would be to make sure that `make test` installs these test dependencies.

Yes, do don't use the legacy resolver anymore 🥳 I'll update the README.

* We lose the versioning of templates in testing, which means that builds are not reproducible: they might pass or fail one day and yield a different result with a different Template version. This is a regression, but I find it an acceptable cost considering that this is an edge case because of the slow change pace of Template.

That is fixable with a bit of tooling, but we can leave that for a next pull request.

However, for now, I think it is an acceptable cost.

* Can you please confirm that this solves #1158, and link it appropriately if that's the case? 🙂

@bonjourmauko
Copy link
Member Author

* Can you please confirm that this solves #1158, and link it appropriately if that's the case? 🙂

Only partially.

There are at least two more pull requests required, one in country, and one in extension, to confirm all works as expected.

@bonjourmauko
Copy link
Member Author

* Can you please confirm that this solves #1158, and link it appropriately if that's the case? 🙂

Only partially.

There are at least two more pull requests required, one in country, and one in extension, to confirm all works as expected.

UPDATE: yes, it actually fixes the aforesaid issue.

@openfisca openfisca deleted a comment from coveralls Nov 17, 2022
@bonjourmauko bonjourmauko force-pushed the elevate-the-constraint-on-packages branch from 8b736f9 to 44b2dc2 Compare November 17, 2022 20:59
@bonjourmauko bonjourmauko force-pushed the elevate-the-constraint-on-packages branch from 44b2dc2 to 5a248d1 Compare November 21, 2022 09:14
@bonjourmauko bonjourmauko force-pushed the elevate-the-constraint-on-packages branch from 5a248d1 to a6244ae Compare November 28, 2022 13:04
@bonjourmauko bonjourmauko force-pushed the elevate-the-constraint-on-packages branch from a6244ae to 8e1507e Compare December 1, 2022 01:09
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.

I do not understand why we remove installing and upgrading pip and twine from the build step, and believe this could lead to errors when pip is too old on the host. However, I will approve this PR to unblock it, on the grounds that CI already tested this to work. I would still appreciate an explanation on the reasoning behind removing these upgrades, which I consider a rider.

@bonjourmauko
Copy link
Member Author

Thanks @MattiSG

@bonjourmauko
Copy link
Member Author

I do not understand why we remove installing and upgrading pip and twine from the build step, and believe this could lead to errors when pip is too old on the host. However, I will approve this PR to unblock it, on the grounds that CI already tested this to work. I would still appreciate an explanation on the reasoning behind removing these upgrades, which I consider a rider.

They are still being run. Yet, as the installation of dependencies was split, without this change they would have been installed twice instead of just once.

@bonjourmauko bonjourmauko merged commit 0caf9fc into master Dec 1, 2022
@bonjourmauko bonjourmauko deleted the elevate-the-constraint-on-packages branch December 1, 2022 16:49
@MattiSG
Copy link
Member

MattiSG commented Dec 13, 2022

For reference, installing of twine is made more properly as an extra dependency group in #1168.

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.

Simplify testing of new major versions
3 participants