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

Add importlib_metadata requirement #1177

Closed
wants to merge 2 commits into from
Closed

Conversation

eraviart
Copy link
Member

Technical changes

@coveralls
Copy link

Coverage Status

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

@eraviart
Copy link
Member Author

eraviart commented Dec 19, 2022

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.

@eraviart eraviart closed this Dec 19, 2022
@MattiSG
Copy link
Member

MattiSG commented Dec 19, 2022

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
Copy link
Member

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 bonjourmauko deleted the importlib_metadata branch December 19, 2022 16:25
@bonjourmauko
Copy link
Member

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 Remove constraint on packages #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing importlib_metadata requirement
5 participants