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

Switch to uv pip compile #2958

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

CoolCat467
Copy link
Contributor

@CoolCat467 CoolCat467 commented Feb 16, 2024

This pull request follows up on #2956 and is attempting to switch the continuous integration system to use astral.sh' new tool uv.
This particular pull request is a subset of #2957 and is in charge of only replacing pip-compile.

This seems nontrivial, can we break this into 2 prs?

1. replace pip-compile

2. replace pip install

Personally I see (1) as much more valuable-- (2) doesn't change much. Personally I'm more excited by uv's ability to get the lowest compatible version so we can add a new CI run than by it replacing pip install.

Originally posted by @A5rocks in #2957 (comment)

@CoolCat467 CoolCat467 added the dependencies Pull requests that update a dependency file label Feb 16, 2024
Copy link

codecov bot commented Feb 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.63%. Comparing base (e87dc46) to head (1748ca9).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2958   +/-   ##
=======================================
  Coverage   99.63%   99.63%           
=======================================
  Files         117      117           
  Lines       17598    17598           
  Branches     3171     3171           
=======================================
  Hits        17533    17533           
  Misses         46       46           
  Partials       19       19           

Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

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

Could you update autodeps too?

babel==2.14.0
# via sphinx
black==24.1.1 ; implementation_name == "cpython"
# via -r test-requirements.in
black==24.1.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, this change seems bugged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made a bug report astral-sh/uv#1429

# -r test-requirements.in
# pytest
exceptiongroup==1.2.0
# via pytest
Copy link
Contributor

Choose a reason for hiding this comment

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

This change too seems wrong

Copy link
Member

Choose a reason for hiding this comment

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

I think it's intended, as we specify --python-version=3.8. If we generate separate test-requirements-pyXX.txt files for each python version we test we'll only get exceptiongroup in the ones that are <3.11

Copy link
Member

Choose a reason for hiding this comment

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

So I think that means we should have a requirements directory wherein we generate txt files for all versions we test.

test-requirements.txt Outdated Show resolved Hide resolved
test-requirements.txt Show resolved Hide resolved
test-requirements.txt Show resolved Hide resolved
# sphinx
# towncrier
markupsafe==2.1.5
# via jinja2
outcome==1.3.0.post0
# via -r docs-requirements.in
Copy link

Choose a reason for hiding this comment

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

Are these expected? or missing compatibility from uv?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uv doesn't appear to explicitly list the fact a dependency is requested from the .in file, which kind of makes sense. If nothing is listed as a what required that module, why waste text? It also makes it more immediately clear when something is a requirement of another package.

Additionally, uv hasn't explicitly said that they are trying to have 100% compatibility for these compiled files, and I personally am for this particular change differing from pip-compile.

Copy link

Choose a reason for hiding this comment

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

If nothing is listed as a what required that module, why waste text?

It'd make senses when multiple .in passed for single output.
Also, it'd be increasing readability for recognizing top-level requirements.

Copy link

Choose a reason for hiding this comment

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

@charliermarsh Do you think this is compatibility problem and need to open issue for it? or this is by design/not planned?

Copy link

Choose a reason for hiding this comment

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

nvm. found the issue: astral-sh/uv#1343

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants