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

refactor: switch from rip to uv #863

Merged
merged 71 commits into from
Mar 1, 2024
Merged

Conversation

tdejager
Copy link
Contributor

@tdejager tdejager commented Feb 26, 2024

Ripped out rip to get uv in. There's a new PyPi 🤠 in town!

It seems to work well, I spent some time on getting the progress to work well.

Some limitations:

  • I broke @nichmor env passing stuf, I don't think its possible with UV yet but we should make a PR.
  • Probably switching python interpreters can create invalid environments, we or uv should check the WheelTags on install.
  • Extra's that determine specific entrypoints aren't handled yet
  • ... everything is still dependent on git sources, we would like the low level libraries on crates.io

Edit:

@ruben-arts ruben-arts added skip-release-notes Will exclude the release notes from automated changelog generation and removed skip-release-notes Will exclude the release notes from automated changelog generation labels Feb 28, 2024
@ruben-arts
Copy link
Contributor

I fixed serialization in: 77b7ea6

pixi add --pypi numpy

resulted in:

[pypi-dependencies]
numpy = { version = "*", extras = [] }

But now just:

[pypi-dependencies]
numpy = "*"

konstin added a commit to astral-sh/uv that referenced this pull request Feb 29, 2024
<!--
Thank you for contributing to uv! To help us out with reviewing, please
consider the following:

- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title?
- Does this pull request include references to any relevant issues?
-->

## Summary

With this PR I've added the option environment variables to the wheel
building process, through the `BuildDispatch`. When integrating uv with
our project pixi (prefix-dev/pixi#863). We ran
into this missing requirement, I've made a rough version here, could
maybe use some refinement.

### Why do we need this?

Because pixi allow the user to use a conda activated prefix for wheel
building, this comes with a number of environment variables, like `PATH`
but also `CONDA_PREFIX` amongst others. This allows the user to use
system dependencies from conda-forge to use during an sdist build.
Because we use `uv` as a library we need to pass in the options
programatically. Additionally, in general there is nothing holding a
python sdist back from actually depending on an environment variable,
see
e.g the test package: https://pypi.org/project/env-test-package/

### What about `ConfigSettings`

I think `ConfigSettings` does not suffice because e.g. CMake could
function differently when the `CONDA_PREFIX` is set. Also, we do not
know if the user supplied backend actually support these settings.

### Path handling

Because the user can now also supply a PATH in the environment map, the
logic I had was the following, I format the path so that it has the
following precedence

1. venv scripts dir.
2. user supplied path.
3. system path.

### Improvements

There is some path modification and copying happening everytime we use
the `run_python_script` function, I think we could improve this but
would like some pointers where to best put the maybe split and cached
version, we might also want to use some types to split these things up.


### Finally

I did not add any of these options to the uv executables, I first would
like to know if this is a direction we would want to go in. I'm happy to
do this or make any changes that you feel would benefit this project.

Also tagging @wolfv to keep track of this as well.  

## Test Plan

<!-- How was it tested? -->

---------

Co-authored-by: konsti <konstin@mailbox.org>
Copy link
Contributor

@ruben-arts ruben-arts left a comment

Choose a reason for hiding this comment

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

Did a speed review, found some unused code that i think has no future value anymore.

src/lock_file/package_identifier.rs Show resolved Hide resolved
src/lock_file/update.rs Outdated Show resolved Hide resolved
@ruben-arts ruben-arts marked this pull request as ready for review February 29, 2024 16:09
@baszalmstra
Copy link
Contributor

@tdejager I updated rattler to the latest main which includes the pep bumps.

@ruben-arts ruben-arts merged commit 0d60798 into prefix-dev:main Mar 1, 2024
14 checks passed
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