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 instead of pip #57

Merged
merged 2 commits into from
Nov 13, 2024
Merged

Use uv instead of pip #57

merged 2 commits into from
Nov 13, 2024

Conversation

fchareyr
Copy link
Contributor

This PR introduces uv as a drop-in replacement for pip.

@fchareyr fchareyr requested a review from a team as a code owner November 13, 2024 16:52
Copy link
Contributor

@mitchnielsen mitchnielsen left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution here @fchareyr

While uv has some proven speed improvements, I wonder if there's any reason we'd want to keep this shared action more generic - any thoughts @jimid27?

action.yaml Show resolved Hide resolved
Copy link
Contributor

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

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

lgtm! just one nit on whether we really need uv run for the prefect deploy commands

Comment on lines +79 to +83
uv run prefect --no-prompt deploy --all --prefect-file "${{ inputs.deployment-file-path }}"
else
IFS=',' read -ra deployment_names <<< "${{ inputs.deployment-names }}"
for name in "${deployment_names[@]}"; do
prefect --no-prompt deploy --prefect-file "${{ inputs.deployment-file-path }}" --name "$name"
uv run prefect --no-prompt deploy --prefect-file "${{ inputs.deployment-file-path }}" --name "$name"
Copy link
Contributor

Choose a reason for hiding this comment

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

are these uv runs necessary? it looks like the setup-uv thing adds the new interpreter to the path already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It adds uv to the path but does not activate the virtual environment. This makes uv run required I believe.

Copy link
Contributor

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

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

holding for failing CI

Copy link
Contributor

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

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

thank you! @fchareyr

@jamiezieziula jamiezieziula merged commit 0276ba3 into PrefectHQ:main Nov 13, 2024
1 check passed
@jimid27 jimid27 mentioned this pull request Dec 6, 2024
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.

4 participants