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

Add override namespace to pyproject.toml/uv.toml #3839

Merged
merged 27 commits into from
Jun 3, 2024

Conversation

Di-Is
Copy link
Contributor

@Di-Is Di-Is commented May 26, 2024

Summary

See #3834 .

This PR adds a new namespace, override-dependencies, to pyproject.toml/uv.toml.
This namespace assumes that the dependencies you want to override are written in the form of requirements.txt.

a example of pyproject.toml

[project]
name = "example"
version = "0.0.0"
dependencies = [
  "flask==3.0.0"
]

[tool.uv]
override-dependencies = [
  "werkzeug==2.3.0"
]

This will improve usability by allowing you to override dependencies without having to specify the --override option when running uv pip compile/install.

Test Plan

added test to crates/uv/tests/pip_compile.rs.

@Di-Is Di-Is marked this pull request as draft May 26, 2024 13:50
Copy link

codspeed-hq bot commented May 26, 2024

CodSpeed Performance Report

Merging #3839 will not alter performance

Comparing Di-Is:add-override-namaspace-to-toml (a25f901) with main (1b769b0)

Summary

✅ 13 untouched benchmarks

@konstin
Copy link
Member

konstin commented May 27, 2024

I skimmed through the diff and it looks good. To get the tests passing you need to run cargo dev generate-json-schema and commit the json change. Mark your PR as ready for review and/or comment when you want a proper review.

@Di-Is
Copy link
Contributor Author

Di-Is commented May 28, 2024

@konstin
Thanks for the advice!
I was able to update the uv.schema.json successfully!

Over the next few days I added a feature to the output of the uv pip compile to show that it was overridden from the workspace configuration.

Here is an excerpt of the output from uv pip compile.

    werkzeug==2.3.0
        # via.
        # --override (from workspace)
        # flask.

However, the implementation has become more complicated with the addition of this feature.
Please review when you have time.

@Di-Is Di-Is marked this pull request as ready for review May 28, 2024 12:54
@Di-Is Di-Is changed the title Add override namaspace to pyproject.toml/uv.toml Add override namespace to pyproject.toml/uv.toml May 28, 2024
crates/pep508-rs/src/origin.rs Show resolved Hide resolved
@@ -17,6 +19,7 @@ impl RequirementOrigin {
match self {
RequirementOrigin::File(path) => path.as_path(),
RequirementOrigin::Project(path, _) => path.as_path(),
_ => panic!("Unsupported RequirementOrigin variant"),
Copy link
Member

Choose a reason for hiding this comment

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

Can we instead attach the file containing the override?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand that multiple toml files(pyproject.toml, uv.toml) are combined to obtain an override dependency.

It seems difficult to keep track of which files have which overriding dependencies.

I'll give it a thought.

Copy link
Member

Choose a reason for hiding this comment

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

Make sense! What i'd like to do is try to avoid explicit panics where possible. Sometimes they are unavoidable, sometimes so you restructure code so the codepath becomes unreachable, sometimes you add a dummy value, etc.

Copy link
Contributor Author

@Di-Is Di-Is Jun 2, 2024

Choose a reason for hiding this comment

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

@@ -25,13 +25,22 @@ impl std::fmt::Display for SourceAnnotation {
RequirementOrigin::Project(path, project_name) => {
write!(f, "{project_name} ({})", path.portable_display())
}
RequirementOrigin::Workspace => panic!("Unsupported RequirementOrigin variant"),
Copy link
Member

Choose a reason for hiding this comment

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

Could you write a value here instead of panicking?

Copy link
Contributor Author

@Di-Is Di-Is Jun 2, 2024

Choose a reason for hiding this comment

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

@@ -37,6 +37,7 @@ pub struct Options {
pub preview: Option<bool>,
pub cache_dir: Option<PathBuf>,
pub pip: Option<PipOptions>,
pub override_dependencies: Option<Vec<String>>,
Copy link
Member

Choose a reason for hiding this comment

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

Can you make this Option<Vec<Requirement>>? This should simplify the specification.rs code too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the deserialization process and JsonSchema and changed the type to Vec<pypi_types::Reuqirement>.

JsonSchema

766bf17#diff-ef8d164011970bb55e80ae99198f0698f74e77faf77cfa76ed6ea650194cad3fR34

Deserialization

766bf17#diff-c615ba811c1d932585ca0c27d737c61469970854c4e527a9e49d73cba5bef35fR46

Comment on lines 110 to 114
// enable warning before workspace loading.
if !cli.global_args.quiet {
uv_warnings::enable();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the flag is not set here, warnings of workspace parsing failure will not be output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed this bug when I was creating the test.

If it is better to separate them into separate PRs, I will remove the relevant process and some of the tests.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, could you create a different PR for those please?

Comment on lines 155 to 160
// Configure the `warn!` macros, which control user-facing warnings in the CLI.
if !globals.quiet {
if globals.quiet {
uv_warnings::disable();
} else {
uv_warnings::enable();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reflect workspace quiet flag after workspace loading.
Added new uv_warnings::disable() for this process.

@Di-Is Di-Is requested a review from konstin June 2, 2024 14:02
@konstin
Copy link
Member

konstin commented Jun 3, 2024

I've removed the warnings changes in this PR to keep this to only adding overrides-dependencies, if you want you can make a separate PR for the warnings changes.

@konstin konstin merged commit 5c77693 into astral-sh:main Jun 3, 2024
46 checks passed
@charliermarsh
Copy link
Member

@konstin - How does this intersect with the existing overrides feature?

@konstin
Copy link
Member

konstin commented Jun 3, 2024

We chain them, so you can use both atm:

// Merge workspace overrides.
let overrides: Vec<UnresolvedRequirementSpecification> = overrides
.iter()
.cloned()
.chain(
overrides_from_workspace
.into_iter()
.map(UnresolvedRequirementSpecification::from),
)
.collect();

@Di-Is Di-Is deleted the add-override-namaspace-to-toml branch July 19, 2024 06:29
charliermarsh pushed a commit that referenced this pull request Jul 21, 2024
Resolves #4467.

## Summary

This PR implements the following

1. Add `tool.uv.constraint-dependencies` to pyproject.toml
1. Support to refer `tool.uv.constraint-dependencies` in `uv lock`
1. Support to refer `tool.uv.constraint-dependencies` in `uv pip
compile/install`

These are analogues of the override features implemented in #3839 and
#4369.

## Test Plan

Add test.
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