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

Enable pre-release install for uv in pyproject.toml #1466

Merged
merged 5 commits into from
Feb 22, 2025

Conversation

wizeng23
Copy link
Contributor

@wizeng23 wizeng23 commented Feb 21, 2025

Description

Unlike pip, uv disallows prelease packages by default, which we need for omegaconf==2.4.0dev3 for oumi>=0.1.5. This PR fixes that in our pyproject.toml.

Related issues

Fixes #1462

Before submitting

  • This PR only changes documentation. (You can ignore the following checks in that case)
  • Did you read the contributor guideline Pull Request guidelines?
  • Did you link the issue(s) related to this PR in the section above?
  • Did you add / update tests where needed?

@@ -31,7 +31,7 @@ envs:

setup: |
set -e
pip install uv && uv pip install oumi[gpu]
pip install uv && uv pip install --prerelease=allow oumi[gpu]
Copy link
Collaborator

@nikg4 nikg4 Feb 21, 2025

Choose a reason for hiding this comment

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

there are a lot of repetitive and verbose changes here. Can this be improved in the future ?

For example, we could make Launcher recognize some special command/macro oumi_pip_install <targets> and
replace oumi_pip_install ... with pip install uv && uv pip install --prerelease=allow ...
so we can avoid such sweeping changes in hundred configs.

This can also make it easier to do changes like this: #1420

Copy link
Collaborator

Choose a reason for hiding this comment

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

That definitely sounds like a doable feature ask. If we scope it out a bit, we can add this as a public issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is no longer needed as I took an alternative approach Oussama suggested

Copy link
Collaborator

@taenin taenin left a comment

Choose a reason for hiding this comment

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

+1 to Nikolai's comment. Consistently sending out PRs updating ~100 files isn't sustainable for reviewing. We should look into a solution to prevent changes like this if possible.

@wizeng23
Copy link
Contributor Author

A PR updating 100 files is unavoidable, as I'll have to do it with Nikolai's suggested change as well. I'm using find-replace to make this change, and am manually review all lines changed before sending it out for review. I understand the concern with this from a reviewing perspective, but IMO creating more convenience wrappers has its own downsides; pip is pretty well known, but if we create oumi_pip, users will now have to search through our code/docs to figure out what it does.

If you really want to avoid any massive code changes, I don't see another way besides doing a pip package release in Omegaconf.

@wizeng23 wizeng23 changed the title Set --prerelease=allow for all uv pip installs Enable pre-release install for uv Feb 22, 2025
@wizeng23 wizeng23 changed the title Enable pre-release install for uv Enable pre-release install for uv in pyproject.toml Feb 22, 2025
@wizeng23
Copy link
Contributor Author

Update: Took an alternative approach Oussama suggested to fix this in the pyproject.toml

@wizeng23 wizeng23 merged commit cc3510d into main Feb 22, 2025
2 checks passed
@wizeng23 wizeng23 deleted the wizeng/uv-prerelease branch February 22, 2025 02:24
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.

0.1.5 depends on unreleased package
4 participants