search

openfisca / openfisca-core

OpenFisca core engine. See other repositories for countries-specific code & data.
https://openfisca.org
GNU Affero General Public License v3.0
168 stars 75 forks source link

Remove constraint on packages #1160

Closed bonjourmauko closed 1 year ago

bonjourmauko commented 1 year ago

Fixes #1158

New Features

bonjourmauko commented 1 year ago

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 commented 1 year ago 
* 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 commented 1 year ago 
* 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.

bonjourmauko commented 1 year ago

Thanks @MattiSG

bonjourmauko commented 1 year ago

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.

MattiSG commented 1 year ago

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