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

Skip uv compile step for URL-based dependencies in add command #817

Closed
wants to merge 1 commit into from

Conversation

cnpryer
Copy link
Contributor

@cnpryer cnpryer commented Feb 29, 2024

Drafting this PR to follow up on #800 debug.

This PR updates the add command to avoid passing URL-based dependencies to uv compile in rye add command.

Testing: Manual + Snapshots

@cnpryer cnpryer changed the title Support path-based dependencies in virtual projects with uv enabled [Question] Support path-based dependencies in virtual projects with uv enabled Feb 29, 2024
@cnpryer cnpryer changed the title [Question] Support path-based dependencies in virtual projects with uv enabled Support path-based dependencies in virtual projects with uv enabled Mar 1, 2024

let mut new_req: Requirement = String::from_utf8_lossy(&rv.stdout).parse()?;
let mut new_req = match req.version_or_url {
Some(VersionOrUrl::Url(_)) => req.clone(),
Copy link
Contributor Author

@cnpryer cnpryer Mar 1, 2024

Choose a reason for hiding this comment

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

I wanted to look into why the requirement with a url would need to make a trip through uv to begin with. I'm sure I'm forgetting something obvious, but this at least demonstrates where the issue might be coming from.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment

Copy link
Contributor

@bluss bluss Mar 4, 2024

Choose a reason for hiding this comment

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

As the code is here, you skip using the result from uv for URLs and that makes sense (If I understand the code correctly) - but should we then also skip using uv at all? I don't know if it's important for the add command do some kind of validation of the arguments like this (checking that they exist)

Edit: ok see now, the command is not spawned in the URL case. So it's already skipping. Seems good to me, but I'm not sure if there are other reasons like I said.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup we seem to be on the same page. Initially it seems like we can just skip it since these are direct references, but I want to understand the big picture a little better.

Thanks for looking at this!

@cnpryer
Copy link
Contributor Author

cnpryer commented Mar 1, 2024

I believe this was expected, but I tried recreating this issue as a uv tests since there are stdin tests over there for pip compile. So far I can't reproduce it.

/// Resolve with PROJECT_ROOT path from a `requirements.in` file on stdin when
/// passed a path of `-`.
#[test]
fn compile_requirements_in_stdin_path() -> Result<()> {
    let context = TestContext::new("3.12");
    let requirements_in = context.temp_dir.child("requirements.in");
    requirements_in.write_str("maturin_editable @ file:///${PROJECT_ROOT}/../../scripts/editable-installs/maturin_editable")?;

    uv_snapshot!(Command::new(get_bin())
    .arg("pip")
    .arg("compile")
    .stdin(fs::File::open(requirements_in)?)
    .arg("-"), @r###"
    success: true
    exit_code: 0
    ----- stdout -----
    # This file was autogenerated by uv via the following command:
    #    uv pip compile -
    maturin-editable @ file:///${PROJECT_ROOT}/../../scripts/editable-installs/maturin_editable

    ----- stderr -----
    Resolved 1 package in [TIME]
    "###);

    Ok(())
}

@cnpryer
Copy link
Contributor Author

cnpryer commented Mar 3, 2024

I'd like to read into URL-based dependencies more before marking this as ready.

In the meantime I'll share my thoughts so far. Since URL-based dependencies are direct references, I'd assume that uv would always emit the same requirements string each time. So if that's the case, I'd think doing this here, where we are just skipping that trip for URL-based dependencies, could work.

I'll give it some more thought.

@cnpryer cnpryer force-pushed the cnp-add-path branch 2 times, most recently from 47d904f to 1031cd1 Compare March 5, 2024 01:44
@cnpryer cnpryer force-pushed the cnp-add-path branch 2 times, most recently from 458ef9d to e373f25 Compare March 5, 2024 23:27
@cnpryer cnpryer changed the title Support path-based dependencies in virtual projects with uv enabled Skip uv compile step for URL-based dependencies in add command Mar 5, 2024
@cnpryer cnpryer force-pushed the cnp-add-path branch 2 times, most recently from 5fe1241 to d366ca5 Compare March 5, 2024 23:33
@cnpryer
Copy link
Contributor Author

cnpryer commented Mar 5, 2024

I'll mark this as ready, but I want to go and review some issues that have been submitted. I think there might be a few this can either begin to address or close completely.

@cnpryer cnpryer marked this pull request as ready for review March 5, 2024 23:40

let mut new_req: Requirement = String::from_utf8_lossy(&rv.stdout).parse()?;
// we can skip the `uv compile` step for dependencies with direct references (URLs/paths)
let mut new_req = if matches!(req.version_or_url, Some(VersionOrUrl::Url(_))) {
Copy link
Member

Choose a reason for hiding this comment

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

Could this be simplified to just:

if matches!(req.version_or_url, Some(VersionOrUrl::Url(_))) {
    continue;
}

At the very top of the loop (line 452)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ty. Spotted that last night and forgot to do it.

@cnpryer
Copy link
Contributor Author

cnpryer commented Mar 6, 2024

A little premature to mark as ready. I need to tweak this, add a --git test, and tag some related issues.

This might warrant a CHANGELOG entry as well.

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.

3 participants