-
Notifications
You must be signed in to change notification settings - Fork 183
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
feat: Add pypi pip requirements export #2049
base: main
Are you sure you want to change the base?
feat: Add pypi pip requirements export #2049
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @synapticarbors This looks good and simple enough. For the tests could you add a test to uv/pip install the file into an environment @abkfenris added one in #2003
We need these tests to be fast.
/// Conda dependencies are not supported in pip requirements files. | ||
/// This flag allows creating the requirements file even if Conda dependencies are present. | ||
#[arg(long, default_value = "false")] | ||
pub ignore_conda_errors: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cannot have a pixi project without conda-dependencies if their are pypi dependencies. So this is a mandatory flag. This logic should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I'll try to make the changes you requested shortly and will hopefully have everything ready today
@ruben-arts -- I just pushed changes that add the in-module insta tests, updated the docs and removed the flag having to do with conda packages. I also added a flag As far as the tests that @abkfenris added in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like your style on the amount of tracing! This is often forgotten!
For the bash test, it should not be to hard to just do an if else on the matrix of the GitHub machines giving a variable that sets the platform name. I guess that would be the quickest way to test them platform specific in CI.
- `--environment <ENVIRONMENT> (-e)`: Environment to render. Can be repeated for multiple envs. Defaults to all environments. | ||
- `--platform <PLATFORM> (-p)`: The platform to render. Can be repeated for multiple platforms. Defaults to all platforms available for selected environments. | ||
- `--ignore-pypi-errors`: PyPI dependencies are not supported in the conda explicit spec file. This flag allows creating the spec file even if PyPI dependencies are present. | ||
- `--split-reqs-no-hashes`: Create a separate requirements.txt for dependencies that do not have an associated hash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand this flag properly. It's default true, but that means setting it will create a false? which would not split the reqs right? If this thought is correct I would rather name it --no-split
or something similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it couldn't the users also want to just ignore the hashes? e.g. --no-hash
? This would then also not require the split.
render_pypi_requirements(target, &base)?; | ||
|
||
if !nohash.is_empty() { | ||
tracing::info!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Info is only shown on --verbose
is that what you want? If it should show on normal level you can either print it with eprintln
or with tracing::warn!
match package { | ||
Package::Pypi(p) => pypi_packages.push(PypiPackageReqData::from_pypi_package(&p)), | ||
Package::Conda(cp) => { | ||
tracing::warn!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this warning is required as it is kind of intended. We should make it clear in the help message and documentation.
// Split package list based on presence of hash since pip currently treats requirements files | ||
// containing any hashes as if `--require-hashes` has been supplied. The only known workaround | ||
// is to split the dependencies, which are typically from vcs sources into a separate | ||
// requirements.txt and to install it separately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to read this part to understand the flag. Could you incorporate this information in the documentation and help of the flag?
false, | ||
), | ||
}; | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// |
(Just dropping in to say thank you in advance @synapticarbors for working on this, as this will be super helpful for me getting |
@matthewfeickert I hope to pick up this PR again at some point, but I'm still not 100% happy with the design. The requirements.txt is sort of complicated in terms of all of the different specifications it supports (including integrating cli flags), issues with how hashes interact with pip, etc. Since I don't tend to use pypi packages, I'm also not sure what most users might want, and how to support most or typical use cases. |
This is a follow-up to #1873 that adds a new
pixi project export pypi-requirements
to export a pip requirements.txt from a pixi lock file. For pixi environments that specify both conda and pypi dependencies, this would allow users topixi project export conda-explicit-spec output --ignore-pypi-errors
together with the new command to create a series of files that could then be installed into a conda env using a combination of conda/mamba and pip/uv.Since the pip requirements file has many valid ways of representing a package as well as flags, I think we side step those complexities by specifying the full url path from the pixi.lock. I'd definitely like feedback from people who work with pip more since typically live fully in conda-land.
One known complexity is that a
requirements.txt
that contains any package with a hash specified is treated as if the--require-hashes
flag has been used for the whole file. This means that you can't mix packages with hashes and without in the samepip install -r ...
. It does appear thatuv
will handle mixed files. To deal with this, I followed the currently recommended workaround and split requirements into two files. There is fix PR in pypa/pip#11968, at least for packages specified from VCS urls, but that's been sitting for a while. As seen for the output of running this on the pypi-source-deps example, this produces:and the outputs look like:
and:
I'll add tests and docs once I get some feedback on the design.