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

Missing importlib_metadata requirement #1176

Closed eraviart closed 1 year ago

eraviart commented 1 year ago

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

Consolidating information from another issue and Slack:

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

MattiSG commented 1 year ago

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

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

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

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