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

Missing importlib_metadata requirement #1176

Closed
eraviart opened this issue Dec 19, 2022 · 5 comments · Fixed by #1178 or #1179
Closed

Missing importlib_metadata requirement #1176

eraviart opened this issue Dec 19, 2022 · 5 comments · Fixed by #1178 or #1179

Comments

@eraviart
Copy link
Member

Since #1165, openfisca_core/taxbenefitsystems/tax_benefit_system.py imports importlib_metadata, but that package is missing from requirements in setup.py.

So every use of a TaxBenefitSystem fails.

@MattiSG
Copy link
Member

MattiSG commented Dec 19, 2022

Consolidating information from another issue and Slack:

  • Reverting Fix documentation #1165 would break the doc, and the changeset is too large to revert as a whole without other side effects.
  • Yanking the failing version from Pypi was requested, but it does not make sense anymore since 3 major versions have already been published on top.
  • I understand a full fix is in Add support for Python 3.8 #1168. That PR was reviewed, with only deployment needing a fix, but it has since then been entirely reworked and force-pushed so it needs a new full review and that will take a longer time since the scope has creeped significantly.
  • I lack the technical understanding of what the failing library importlib_metadata does precisely, but from its name and where it was introduced, it seems to only be used for obtaining the package name when initialising a TaxBenefitSystem. This was previously taken from pkg_resources, so that can maybe be restored, or an alternative package can be found. As worst case, perhaps an alternative implementation to obtain the necessary metadata can be made.

@maukoquiroga @benjello you respectively authored and approved the changeset that introduced the regression, can you fix this?

@MattiSG
Copy link
Member

MattiSG commented Dec 19, 2022

We also have to understand, as a postmortem, what is missing in the test suite that enabled several versions to be published with such a major problem.

@bonjourmauko
Copy link
Member

bonjourmauko commented Dec 19, 2022

@MattiSG I will push a fix, yet it is IMHO incorrect to label it as a regression, as it is a problem caused by an external change in dependencies (it gets frustrating by the day, I know).

In fact, I think just reverting would still make flake8 to fail because of incompatible versions with twine (keyring) as @eraviart reported. By the way thanks @eraviart for #1177

It's hard to provide an analytical postmortem at this point, as most of what is happening seems related to poor dependency management and very old code (pkg_ressources use had to be refactored).

Concerning #1168, it includes one possible fix for this, but changes were requested because of the publishing job failing (thanks for testing) so it invariably requires a new review.

Appart from the manual workflow dispatch configuration, the scope of that PR has actually been reduced. The reason I chose to split the workflows was to make it easier to spot bugs like the publishing one that you reported.

I understand the frustration of splitting the files, however I wouldn't feel comfortable integrating the version as it was first reviewed. Yet, extracting the changes concerned with the reported error is easy 😃

@bonjourmauko
Copy link
Member

We also have to understand, as a postmortem, what is missing in the test suite that enabled several versions to be published with such a major problem.

I think the best solution is to manage build, test and deploy dependencies in isolation. #1161 goes in that direction.

However, it seems unlikely to avoid this 100%, but that would make a great difference IMHO.

@bonjourmauko
Copy link
Member

After taking a closer look, #1165 did actually broke builds under some configurations, in particular in python versions 3.8+, the culprit being a failure to declare a dependency that was being overridden by others in a least two scenarios:

  1. Installing dependencies outside of the setup.py that had importlib-metadata as a transitive dependency
  2. Using the deprecated legacy resolver to install dependencies.

The direct postmortem I can draw now, with a better distance from the issue:

  1. Using libraries without having them declared in an explicit way in the setup.py
  2. Not testing in CI against the affected versions of python
  3. Installing libraries outside of the setup.py
  4. Caching together libraries inside and outside of the setup.py

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