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

Implemented first version of uv executor #271

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

AKuederle
Copy link

Description of changes

This PR implements a uv-executor that uses uv run to execute tasks against the virtual env. The advantage over executing uv projects with the normal venv executor is, that uv run will automatically update and sync packages and is hence, recommended over activating the venv manually when working with uv.

@nat-n This is a first version, and I just wanted to get your feedback, if this is something you would be interested in merging.

See also #257

Pre-merge Checklist

  • New features (if any) are covered by new feature tests
  • New features (if any) are documented
  • Bug fixes (if any) are accompanied by a test (if not too complicated to do so)
  • poe check executed successfully
  • This PR targets the development branch for code changes or main if only documentation is updated

description = "Add your description here"
readme = "README.md"
requires-python = ">=3.12"
dependencies = []
Copy link
Owner

@nat-n nat-n Jan 13, 2025

Choose a reason for hiding this comment

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

Do you think it's worth declaring a dependency here to exercise the integration more fully?

If yes there's a locally vendored package you can use like

cowpy = { path = "../packages/cowpy-1.1.5-py3-none-any.whl" }

@nat-n
Copy link
Owner

nat-n commented Jan 13, 2025

Hey @AKuederle, this looks pretty good. Thanks for working on it!

In the poetry executor I prefer using the venv directly, mostly to avoid the latency of the poetry CLI, but I guess that's not such an issue with uv :)

@AKuederle
Copy link
Author

Yes I think the entire idea behind uv run is that running the additional checks is so fast, that it does not matter.
We might need to add a fallback (as I don't know what happens in cases where you don't have internet).

Regarding tests: I wasn't sure what tests to add. Based on my understanding, by adding a new project folder it should be included in the fixture. But I am not sure if this is sufficient. I also got some failing tests that did not seem to be related to my changes.

Could you provide some pointers, what I should update add, for you to consider merging this?

@nat-n
Copy link
Owner

nat-n commented Jan 14, 2025

I'm curious how you would make an offline fallback work (if it's needed)?

I'm afraid the testing setup isn't quite that clever. Given the fixture project you created you'll need a test using run_poe_subproc, for example: run_poe_subproc("test-package-exec-version", project="uv").

Most tests work the same way, you can look here for example: https://github.com/nat-n/poethepoet/blob/main/tests/test_executors.py You can either extend that test module or create a new more specific one.

If a test takes more than a couple of seconds to run (for example due to needed to setup the venv for the uv project) then it should be marked with @pytest.mark.slow, and if it requires the uv executable to be present to pass then it should be marked with @pytest.mark.skipif(not shutil.which("uv"), reason="No uv available")

I see a test failing due to a recent unreleased change I made. I pushed a fix to the development branch so you can rebase to get it.

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.

2 participants