-
Notifications
You must be signed in to change notification settings - Fork 3k
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/hatch instead of Poetry #3039
Conversation
Just for visibility, I'm working from this draft PR here https://github.com/locustio/locust/pull/3039/files
Probably going to be next week before I tackle the pipeline, as this takes longer generally, but I'd say it's going well 🥳 |
Ok the CI is fully green on this 🚀 💯 Things done:
Things not done:
|
Looks grand! Going through the code now! |
.github/workflows/tests.yml
Outdated
cache: 'yarn' | ||
cache-dependency-path: locust/webui/yarn.lock | ||
# cache: 'yarn' | ||
# cache-dependency-path: locust/webui/yarn.lock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the reason to change this? (this is probably mostly @andrewbaldwin44 's area :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm gonna reinstate this --- it was originally breaking something but it's on my TODO listed here: #3039 (comment)
.github/workflows/tests.yml
Outdated
|
||
- run: uv venv --python 3.11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setting version here is probably redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I believe this was maybe because the original CI is running under 3.11
at a basic level, if we don't specify, I think it will default to 3.12
---
Whatever you wanna pick I'd just like to make sure that's the same everywhere that doesn't have a specific version required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it default to the version installed by actions/setup-python (specified on line 38)?
If it does default to that then I prefer not specifying it again, as someone is bound to forget to update both places :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good shout, I'll start taking a note of these on the TODO
Hmm... I just did
Activating the venv and running locust directly works fine.
|
@cyberw yes - running The reason For example, this works: |
I guess I still have @andrewbaldwin44 's comments about |
How come it worked for poetry? Can we fix how we use relative paths? We've gotten some other semi-related complaints too: https://github.com/orgs/locustio/discussions/3040 |
If I fix the issues with html.py (by temporarily modifying sys.path to not include locust), I can change the import in |
This is similar to what I do in my projects --- As you said, doing that just requires the package base ( |
Up to you, I could do this on a branch-of-this-branch and take a look at how it feels I guess, it's obviously a bigger, potentially breaking change I'm not sure if I was hoping the |
This makes sense, maybe it would avoid this type of issue also, using absolute paths |
I will fix that, once this PR has been merged (if this is enough, I prefer not making a bigger change) |
Hey @cyberw this should be good for a review. Couple things
It's shaving 2-3 mins off the e2e CI runs as well which is nice 🥳 new vs old |
docs/developing-locust.rst
Outdated
|
||
.. code-block:: console | ||
$ tox | ||
$ hatch run +py=3.10 test:all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess there's no direct equivalent, that runs all versions in one invocation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically we should be able to wrap it up in hatch test
re: this
Originally this seemed to just break when collecting tests with pytest
- hatch test
just appears to be a way of calling pytest
specifically
I'll mess around with it and see if I can get it to work
docs/developing-locust.rst
Outdated
|
||
.. code-block:: console | ||
$ pytest -vv locust/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it really make sense to add -vv everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary 👍 it's just a tad easier to see what specific tests are breaking during execution, can just omit this by default
Awesome! Apart from those two nit-picks, I think we're good to go!
Sorry, I dont quite follow? Anyway, I didn't see anything that looked terrible :) And docs are easy to fix "later"... |
Nothing in particular 🥳 |
Updated this again, I've layered some of the hatch environments a bit to leverage the default "hatch test" environment The result is that:
|
Looking real good! In your perspective, is it good enough to merge? Let me know a time that works for you (I know the builds are working on your branch, but only merging to master will tell us for sure that the pypi/docker upload still works and gets the right versions :) |
I'm with you, this is everything obvious I can think of, it just needs ironed out in prerelease now I'll be around here and there at the weekend, but I can set aside time on Monday to play whack-a-mole if that works? |
Monday sounds great! |
I keep getting this warning:
Probably we just need to add |
Yeah I think that's it, I'll stick that in the config. |
Cool! I’ll hit merge in about 5 minutes :) |
Tracking WIP towards replacing
poetry
with uv/Hatch (and maybe ditching tox at the same time)