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

build(deps): avoid version conflicts #636

Merged
merged 28 commits into from
May 24, 2023
Merged

Conversation

qued
Copy link
Contributor

@qued qued commented May 23, 2023

Addresses #631.

  • Uses constraints to keep dependency versions more consistent.
  • Moves all dependencies to .in files which are then ingested by setup.py.
  • Adds script to check consistency of all extras.
  • Adds consistency check to CI.

I should note that while it shouldn't be possible to cause a conflict between base.txt and any of the extras (because base.txt constrains all the extras) it is possible to get a conflict between two of the extras files. There are ways of trying to avoid that (like constraining each file by all the files that have already been processed before it in the order given in the make pip-compile target) but the ones I could think of seemed a little overwrought, and come with problems of their own. If a conflict arises, it should be flagged by CI or locally with make check-deps. When/if that happens, you can resolve the conflict by adding appropriate global constraints in requirements/constraints.txt.

Also note that if fileA.in is constrained by fileB.txt, then fileB.in should be compiled before fileA.in in the make pip-compile target. Otherwise fileA.in will be compiled with the old version of fileB.txt which can cause conflicts or keep dependencies from being updated properly.

Testing:

  • Verify make check-deps says deps are consistent.
  • Change line 59 of requirements/ingest-s3.txt to:
urllib3==1.27.1
  • Verify make check-deps produces an error.

@qued qued requested a review from cragwolfe May 23, 2023 23:51
@cragwolfe
Copy link
Contributor

Flagging @ryannikolaidis in case he is aware of any potential downstream issues with the urllib<2 pin. But, really great to see the dep issues sorted out and a way of preventing the mismatches in the future. Passing CI with Dockerfile is also promising 🤞 .

@qued qued marked this pull request as ready for review May 24, 2023 00:26
@ryannikolaidis
Copy link
Contributor

Flagging @ryannikolaidis in case he is aware of any potential downstream issues with the urllib<2 pin. But, really great to see the dep issues sorted out and a way of preventing the mismatches in the future. Passing CI with Dockerfile is also promising 🤞 .

Thanks! Nope, I don't think we had anything explicitly forcing our hand. iirc the main push to urllib3 was because the latest for a bunch of our dependencies were requiring urllib3>2. As long as we're okay with those not getting bumps, then I don't think there are any other immediate concerns.

Thanks for this @qued! 🙏

Copy link
Contributor

@cragwolfe cragwolfe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@qued qued enabled auto-merge (squash) May 24, 2023 22:28
@qued qued merged commit c82bad1 into main May 24, 2023
@qued qued deleted the build(deps)/avoid-version-conflicts branch May 24, 2023 22:29
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.

3 participants