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
165 stars 74 forks source link

Add importlib_metadata requirement #1177

Closed eraviart closed 1 year ago

eraviart commented 1 year ago

Technical changes

coveralls commented 1 year ago

Coverage Status

Coverage remained the same at 77.659% when pulling 914b86f47bd4cfb65965f57de4f2ebdea4b8ba75 on importlib_metadata into 441adaccc90175b0fd96dfecaa5da095437af1e0 on master.

eraviart commented 1 year ago

I'm not able to find an importlib_metadata version that meets the constaints of the requirements of flake8, keyring, etc. So I delete this PR. As a quick fix I suggest to remove the use of importlib_metadata and to revert at least partially #1165 that introduced it.

MattiSG commented 1 year ago

Translation of the above message:

I can't find a version of importlib_metadata that respects the constraints of flake8 and other keyring requirements. So I'm deleting this PR and, for lack of anything better, I'm arguing for the deletion of the use of importlib_metadata and the at least partial reverting of #1165 which introduces it.

Reverting #1165 would break the doc.

I already requested yanking the failing version from Pypi.

I understand a full fix is in #1168. That PR was ready to be shipped last week, with only deployment needing a fix, but it has since then been entirely reworked and force-pushed so it needs a new full review 🙃

I'm arguing for the deletion of the use of importlib_metadata

I do not know what is the use of this module. Can you clarify what would be the consequences?

bonjourmauko commented 1 year ago

The problem comes from a version change in twine, reverting nor yanking would solve the problem. This is not a bug introduced by the mentioned PR.

bonjourmauko commented 1 year ago

Technical changes

* Add missing requirement `importlib_metadata` imported by `openfisca_core/taxbenefitsystems/tax_benefit_system.py` (Fix [Missing importlib_metadata requirement #1176](https://github.com/openfisca/openfisca-core/issues/1176) ; see also [comment of issue 1165](https://github.com/openfisca/openfisca-core/pull/1165#issuecomment-1332299948))

This is the good fix, I completed it in #1138 💯

I'm not able to find an importlib_metadata version that meets the constaints of the requirements of flake8, keyring, etc. So I delete this PR. As a quick fix I suggest to remove the use of importlib_metadata and to revert at least partially #1165 that introduced it.

Thanks @eraviart . importlib_metadata is an officially recommended replacement of pkg_resource. The root cause of the problem, are two:

  1. We (I) relied on a transient dependency by omission, and conflicting requirements broke the build.
  2. We're spreading bad practices from the country_template: no dependencies should be installed out of the setup.py in the same env. Now pip ships with a dependency resolver that we can correctly use since #1160.

We've had countless bugs already due to the same dependency resolution problem. #1015 is one possible definite solution to this, yet this is a known source of bugs in general.

See in #1179 I added idna: a new release of openapi-spec-validator relies on a library that they only declared as a dev dependency, so I got a new bug while trying to solve this one :/

I think the incorporation of a lock-file with automatic env management imposes itself, I don't see another way to minimise this issues arising in the future (we can't fully stop them, as the openapi-spec-validator shows).

I already requested yanking the failing version from Pypi.

I missed that message, sorry. But it would've created another problems. And the fix is non-functional: the Makefile we propose in country-template has to be changed to avoid propagating this issues further down.

I understand a full fix is in #1168.

A full yet non definitive fix is in #1179.

That PR was ready to be shipped last week, with only deployment needing a fix.

If it needed a fix it was not ready to be deployed, and glad you found it!

but it has since then been entirely reworked

I interpreted the failing deployment as coming from the increased complexity of the workflow.yaml file. In fact, the splitting of the workflows —something we evoked during pair programming— allowed me to spot it.

While the changes might be rolled-back, I'd advise not to and instead give it a full new review.

I did easily extract #1179 however.

and force-pushed so it needs a new full review 🙃

I can't find trace of that force-push, apologies if that was the case but I'm more confident it was not the case.

I'm arguing for the deletion of the use of importlib_metadata

I do not know what is the use of this module. Can you clarify what would be the consequences?

importlib_metadata is an official backport of importlib.metadata, an official replacement of pkg_resources.