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

Use UV for package install and dependency resolution #1016

Merged
merged 15 commits into from
Aug 12, 2024
Merged

Conversation

bmos
Copy link
Contributor

@bmos bmos commented Mar 14, 2024

What

uv is "an extremely fast Python package installer and resolver, written in Rust."

Why

Your macOS testing is significantly limited by only testing with python 3.8. Based on the comments in your actions, it seems like this is because macOS actions are much more expensive (and unfortunately that workflow takes even longer to run on macOS than it does on Windows). If the macOS workflow was able to execute faster, you could run more of them without incurring as much additional usage or just save your usage generally.

Your current workflow step of installing dependencies results in a notice: INFO: This is taking longer than usual. You might need to provide the dependency resolver with stricter constraints to reduce runtime.

Both of these concerns can be addressed by using uv. Using the included linting workflow takes far less time per job compared to the workflow you're using now.

Why Not

uv is very new and relatively unknown compared to pip and does have some behavior differences from pip (such as how it does not default to installing yanked versions -- which could be a positive but is a noteworthy difference).

uv doesn't support python 3.7, which PyPI says parsons does (although parsons tests don't pass under 3.7)

@bmos bmos marked this pull request as ready for review March 14, 2024 11:33
@bmos bmos changed the base branch from major-release to main April 7, 2024 17:57
@bmos bmos changed the base branch from main to new-release April 7, 2024 18:08
@bmos bmos changed the base branch from new-release to major-release April 7, 2024 18:08
@shaunagm
Copy link
Collaborator

shaunagm commented Apr 18, 2024

Hi @bmos! Thanks for this PR. It looks like it decreases the Mac runs from about 3m each to 1m. I do wonder whether adding Python 3.11 and 3.12 to the matrix, which we plan to do soon, might cause usage issues again. I don't actually have much visibility into how close we are to the limit for the free tier, so I'll look into that. In the meantime, @bmos are you in our Slack? We'd love for you to join the community there. :)

@bmos
Copy link
Contributor Author

bmos commented Apr 18, 2024

Just emailed asking for an invite. Thanks for the nudge.

I'd say if 3.8-3.12 is going to be shortly supported, it might make sense to test 3.8, 3.10, and 3.12 on Linux and 3.8 and 3.12 on macOS. Or if Linux runs are basically unlimited just test them all there and just those 2 on macOS.
The most important tests are the oldest and newest versions as they're the ones most likely to support some syntax+dependencies and not others.

@bmos
Copy link
Contributor Author

bmos commented Jun 21, 2024

I followed up with another email in May and am about to send another.
Am I emailing the wrong place? engineering+parsons [at] movementcooperative.org ?

@shaunagm
Copy link
Collaborator

Not sure why no one's responding to that email but you can just email me directly at [email protected]. Sorry!

@bmos bmos changed the base branch from major-release to main June 25, 2024 20:26
@bmos
Copy link
Contributor Author

bmos commented Jun 25, 2024

I'm rebuilding this PR against main, based on our discussion today.

@bmos bmos force-pushed the uv branch 3 times, most recently from eea6e39 to 9aa9572 Compare June 28, 2024 12:25
@bmos
Copy link
Contributor Author

bmos commented Jun 28, 2024

Rebuilt it again to resolve merge conflicts after 3.12 updates.
The workflows checks are passing on my fork
https://github.com/bmos/parsons/actions/runs/9894790720

@bmos
Copy link
Contributor Author

bmos commented Jul 11, 2024

i have added a test which ensures that install works via pip
it only runs when requirements, setup.py, or the workflow itself is updated
https://github.com/bmos/parsons/actions/runs/9894785213

i'm not sure whether it needs to run for every OS/python version pair that we run the usual tests on, so I'd love feedback on that question. if it does, it seems to have removed much of the benefit of this PR (well not removed the benefits but introduced a drawback: CI complexity)

@shaunagm
Copy link
Collaborator

Can you clarify/rephrase? I'm not sure I understand this question:

i'm not sure whether it needs to run for every OS/python version pair that we run the usual tests on, so I'd love feedback on that question. if it does, it seems to have removed much of the benefit of this PR (well not removed the benefits but introduced a drawback: CI complexity)

@bmos
Copy link
Contributor Author

bmos commented Jul 17, 2024

In test-pip-install.yml, the same set of python versions is defined as in the linting/testing script.
Having test/lint/pip install not share a single source of truth feels messy.
This is what I'm wondering about.

Ideally we probably also shouldn't be checking the formatting multiple times.

@shaunagm
Copy link
Collaborator

shaunagm commented Jul 18, 2024

Github Actions may have a way to define once and then import.

But we could probably get away with testing on the lowest and highest version of Python on each OS.

@shaunagm
Copy link
Collaborator

shaunagm commented Aug 8, 2024

So I'd like to get this merged but there may be some conflicts with #1108. I guess once that is merged we can see what comes up?

I think for now we can merge and if we end up overdrawing our github actions minutes we can scale back the number of tests we run.

@bmos
Copy link
Contributor Author

bmos commented Aug 9, 2024

@bmos
Copy link
Contributor Author

bmos commented Aug 9, 2024

Great! I got the tests passing again.

@shaunagm
Copy link
Collaborator

Thanks @bmos! The switch from Black/Flake8 to Ruff has been merged, which unfortunately but not unexpectedly introduced some conflicts.

@bmos
Copy link
Contributor Author

bmos commented Aug 12, 2024

Ok! Happy to fix it up again shortly

@bmos bmos force-pushed the uv branch 2 times, most recently from 3800731 to 49b610e Compare August 12, 2024 15:30
@bmos
Copy link
Contributor Author

bmos commented Aug 12, 2024

Ok I think this is done now 🥳

@bmos
Copy link
Contributor Author

bmos commented Aug 12, 2024

macOS + python 3.8 seems to take a long time because it needs to compile one of the dependencies, but it is still faster.

@bmos
Copy link
Contributor Author

bmos commented Aug 12, 2024

I moved the test order to lint, test, format.
That way issues that would be caught quickly by linting can be caught before wasting power/time on testing and issues that are more serious than formatting can be caught before formatting issues (since failure stops any later steps from running).

@shaunagm
Copy link
Collaborator

Our old linter (flake8) had overlap with the formatter - do we know if that's the case again here? We don't want a situation where the linter is failing on style issues that the formatter would fix.

(Caveat: I am still kind of confused about the scope of the linter vs the formatter, so it's possible my question doesn't make sense.)

@bmos
Copy link
Contributor Author

bmos commented Aug 12, 2024

That's a good question, I'll dig into ruff's documentation.
The two commands that run ruff are still executed in the same order that they were in the ruff pr, but I do understand your concern.

@bmos
Copy link
Contributor Author

bmos commented Aug 12, 2024

It appears that the two are supposed to be able to be used without overlap:
astral-sh/ruff#9064

@bmos
Copy link
Contributor Author

bmos commented Aug 12, 2024

I added the check flag so that it would not actually be reformatting the files and would exit with a failed status. I think that as written the formatter action step would always show a pass even if there were files that needed to be reformatted.

@shaunagm shaunagm merged commit 6c78541 into move-coop:main Aug 12, 2024
30 checks passed
@bmos bmos deleted the uv branch August 12, 2024 20:50
@shaunagm
Copy link
Collaborator

shaunagm commented Sep 3, 2024

Did this change accidentally remove our Windows tests? @bmos can you confirm? (If so, this is on me, I should have caught that)

@bmos
Copy link
Contributor Author

bmos commented Sep 3, 2024

No, it was already commented out. See the diff.

@bmos
Copy link
Contributor Author

bmos commented Sep 3, 2024

But if it's supposed to work on Windows we should absolutely add that.

@shaunagm
Copy link
Collaborator

shaunagm commented Sep 3, 2024

Oh jeez I could swear we added Windows tests but I'm finding no evidence in our PR history so maybe I just hallucinated it. Definitely adding Windows tests is a high priority.

@jdw25
Copy link
Collaborator

jdw25 commented Sep 5, 2024

Would it make sense for "Adding Windows tests" to be its own issue?

@bmos
Copy link
Contributor Author

bmos commented Sep 5, 2024

I didn't make an issue but I have this mostly done already in a draft PR
#1119

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