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.

Sign up for GitHub

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

Jump to bottom

chore: switch from pipenv to uv #232

Merged
merged 1 commit into from
Dec 11, 2024
Merged

chore: switch from pipenv to uv #232

merged 1 commit into from
Dec 11, 2024

Conversation

leseb
Copy link
Collaborator

@leseb leseb commented Dec 9, 2024

uv from the people who wrote ruff is an extremely fast Python package and project manager.

Description

5989502 chore: switch from pipenv to uv

commit 5989502
Author: Sébastien Han [email protected]
Date: Mon Dec 9 15:33:46 2024 +0100

chore: switch from pipenv to uv

uv from the people who wrote ruff is an extremely fast Python package
and project manager.

Signed-off-by: Sébastien Han <[email protected]>

How Has This Been Tested?

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@leseb leseb requested a review from tumido December 9, 2024 14:39
@leseb leseb marked this pull request as ready for review December 9, 2024 14:39
tumido
tumido reviewed Dec 9, 2024
View reviewed changes
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

Love it. ❤️ However we should announce that this PR would require everybody to migrate from pipenv to uv. It also changes how you work with the repo depending on if you checkout to phase-1/phase-2 or main...

Maybe we should keep Pipfile/Pipfile.lock around for compatibility sake and advice to use uv in the future. And we should also consider backporting to phase-1/phase-2 before removing Pipenv from the picture?

@leseb
Copy link
Collaborator Author

leseb commented Dec 9, 2024

Love it. ❤️ However we should announce that this PR would require everybody to migrate from pipenv to uv. It also changes how you work with the repo depending on if you checkout to phase-1/phase-2 or main...

Correct, I'm planning backporting this once we agree to merge it.

Maybe we should keep Pipfile/Pipfile.lock around for compatibility sake and advice to use uv in the future. And we should also consider backporting to phase-1/phase-2 before removing Pipenv from the picture?

While keeping Pipfile/Pipfile.lock around for compatibility seems fine, maintaining two dependency management tools would introduce overhead. Given the relatively small contributor base, I lean towards a clean break from Pipenv. Breaking early will encourage contributors to adopt uv sooner and reduce the risk of confusion or delays. I can write a small contributing guide that reflect how to build a venv with uv.

What do you think?

@tumido
Copy link
Member

tumido commented Dec 10, 2024

I'm fine with that... I can manage the slight inconvenience changing the venv provider when rebasing my PRs (in case my branch is on Pipenv and main is on uv with different venvs...). Not sure how much inconvenient it will be for others. 🙂

Clean break is fine with me, I would love to hear what others think. Maybe having a simple quick migration guide would be helpful. I see multiple options for that as well:

  • a markdown doc in repo / README entry (which we would need to clean up after some time, once it's no longer relevant)
  • as a documentation in this PR linked from README or something
  • a separate GH issue that we can pin of the time being and retire once we are assured everybody has migrated...

Either option is fine. Whatever is easier to manage. 🙂

@leseb
Copy link
Collaborator Author

leseb commented Dec 10, 2024

I'm fine with that... I can manage the slight inconvenience changing the venv provider when rebasing my PRs (in case my branch is on Pipenv and main is on uv with different venvs...). Not sure how much inconvenient it will be for others. 🙂

Clean break is fine with me, I would love to hear what others think. Maybe having a simple quick migration guide would be helpful. I see multiple options for that as well:

  • a markdown doc in repo / README entry (which we would need to clean up after some time, once it's no longer relevant)
  • as a documentation in this PR linked from README or something
  • a separate GH issue that we can pin of the time being and retire once we are assured everybody has migrated...

Either option is fine. Whatever is easier to manage. 🙂

In terms of migration, there is not much to do despite installing uv and getting familiar with the CLI by going through their docs. Do we really need to write something for this?

@leseb leseb requested a review from tumido December 10, 2024 15:30
@tumido
Copy link
Member

tumido commented Dec 10, 2024

I guess it can be as simple as adding something like this to the README? WDYT?

## Developer setup

To collaborate on this repository, please follow these steps:
1. Install [uv](https://docs.astral.sh/uv/getting-started/installation/)
2. Run following commands to prepare your local environment
   ```
   uv venv && uv sync
   ```

@tumido
Copy link
Member

tumido commented Dec 10, 2024

In addition we need to communicate that the day to day workflow changes from pipenv shell to source .venv/bin/activate. 🤷 🙂

tumido
tumido reviewed Dec 11, 2024
View reviewed changes
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

Just a few README suggestions. This should be the last time, I promise! 😄 🤷

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@leseb @tumido
chore: switch from pipenv to uv
be19a32 
uv from the people who wrote ruff is an extremely fast Python package
and project manager.

Co-authored-by: Tomas Coufal <[email protected]>
Signed-off-by: Sébastien Han <[email protected]>
@leseb leseb requested a review from tumido December 11, 2024 10:21
@leseb
Copy link
Collaborator Author

leseb commented Dec 11, 2024

Just a few README suggestions. This should be the last time, I promise! 😄 🤷

That's ok, I don't mind the back and forth :-)

tumido
tumido approved these changes Dec 11, 2024
View reviewed changes
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

Like it. 👍 Let's announce it before merging. Whenever you feel like it. 🙂

/lgtm
/approve

@leseb
Copy link
Collaborator Author

leseb commented Dec 11, 2024

Like it. 👍 Let's announce it before merging. Whenever you feel like it. 🙂

/lgtm /approve

Done on Slack, thanks!

@leseb leseb merged commit 116ead6 into opendatahub-io:main Dec 11, 2024
1 check passed
@leseb leseb deleted the uv branch December 11, 2024 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tumido tumido tumido approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@leseb @tumido