Normalize extras in lockfile #3958

charliermarsh · 2024-06-01T18:13:57Z

Summary

Previously, when we locked something like flask[dotenv], we created two separate distributions in the lockfile: one for flask, which included the base dependencies, and one for flask[dotenv], which included the base dependencies and the dotenv dependencies. This was easy to implement, but it meant that we were duplicating all of the distribution files for every extra, and duplicating all of the base dependencies for every extra.

This PR normalizes the data such that we now have one entry per distribution (i.e., ExtraName was removed from DistributionId), with an optional dependencies table with an entry per extra, like:

[[distribution]]
name = "project"
version = "0.1.0"
source = "editable+file://[TEMP_DIR]/"
sdist = { url = "file://[TEMP_DIR]/" }

[[distribution.dependencies]]
name = "anyio"
version = "3.7.0"
source = "registry+https://pypi.org/simple"

[distribution.optional-dependencies]

[[distribution.optional-dependencies.test]]
name = "iniconfig"
version = "2.0.0"
source = "registry+https://pypi.org/simple"

This requires a bit more work upfront, because we now need to merge multiple packages from the PetGraph representation when creating the lockfile.

Closes #3916.

charliermarsh · 2024-06-01T18:17:40Z

crates/uv-resolver/src/lock.rs

    pub(crate) source: Source,
 }

 impl DistributionId {
-    fn from_annotated_dist(annotated_dist: &AnnotatedDist) -> DistributionId {
+    pub(crate) fn from_annotated_dist(annotated_dist: &AnnotatedDist) -> DistributionId {


Not thrilled that I'm making so many things pub(crate) here. Should I change ResolutionGraph::to_lock into a impl TryFrom<ResolutionGraph for Lock?

I kind of bias toward concrete and bespoke (but conventional) conversion routines like ResolutionGraph::to_lock unless there's a specific need for generic fallible conversions.

I think this is more of a stylistic choice, but I'd say it's just a specific manifestation of "don't go generic unless there's a reason to." And with specific conversion routines, it's straight-forward to add more parameters if they ever become necessary.

Ooooooooo, I completely misunderstood your question. You were suggesting the TryFrom impl so that the conversion would be defined in this module, and that would in turn prevent exposing stuff.

Yeah I think I'd do that. Although, following from my previous comment, I'd probably just define Lock::from_resolution_graph or something. And that might in turn require exposing more stuff from the graph, but maybe that's okay.

(I don't think we've really settled on a great balance here. I wonder, for example, whether it really makes sense to have a ResolutionGraph at all. But this gets to the "installation might want different types" idea that's been floating around. It's a bigger refactor for sure.)

Yeah I'm also not totally convinced that ResolutionGraph will be necessary in the long run.

charliermarsh · 2024-06-01T18:17:57Z

@BurntSushi - Ignoring the code, curious if you prefer this representation?

codspeed-hq · 2024-06-01T18:26:05Z

CodSpeed Performance Report

Merging #3958 will not alter performance

_{Comparing charlie/ex (02f3ead) with main (362b00c)}

Summary

✅ 13 untouched benchmarks

ibraheemdev

The new representation generally makes sense to me.

Would it make more sense to put optional dependencies under distributions.extras."name".dependencies? Could we ever want to put other information under distributions.extras."name"?

ibraheemdev · 2024-06-03T16:41:40Z

crates/uv-resolver/src/resolution/graph.rs

            let mut locked_dist = lock::Distribution::from_annotated_dist(dist)?;
            for neighbor in self.petgraph.neighbors(node_index) {
                let dependency_dist = &self.petgraph[neighbor];
                locked_dist.add_dependency(dependency_dist);
            }
-            locked_dists.push(locked_dist);
+            if let Some(locked_dist) = locked_dists.insert(locked_dist.id.clone(), locked_dist) {


Why could the previous code do an unconditional push here?

BurntSushi

I think I'd echo @ibraheemdev's question. Otherwise, this generally LGTM. If it's possible, I think I would prefer a way where we're not slapping pub(crate) on everything, but I don't feel too strongly at this point while we're still trying to figure out what the data types should be.

BurntSushi · 2024-06-03T17:16:56Z

crates/uv-resolver/src/lock.rs

    pub(crate) source: Source,
 }

 impl DistributionId {
-    fn from_annotated_dist(annotated_dist: &AnnotatedDist) -> DistributionId {
+    pub(crate) fn from_annotated_dist(annotated_dist: &AnnotatedDist) -> DistributionId {


I kind of bias toward concrete and bespoke (but conventional) conversion routines like ResolutionGraph::to_lock unless there's a specific need for generic fallible conversions.

I think this is more of a stylistic choice, but I'd say it's just a specific manifestation of "don't go generic unless there's a reason to." And with specific conversion routines, it's straight-forward to add more parameters if they ever become necessary.

BurntSushi · 2024-06-03T17:19:41Z

crates/uv-resolver/src/lock.rs

    pub(crate) source: Source,
 }

 impl DistributionId {
-    fn from_annotated_dist(annotated_dist: &AnnotatedDist) -> DistributionId {
+    pub(crate) fn from_annotated_dist(annotated_dist: &AnnotatedDist) -> DistributionId {


Ooooooooo, I completely misunderstood your question. You were suggesting the TryFrom impl so that the conversion would be defined in this module, and that would in turn prevent exposing stuff.

Yeah I think I'd do that. Although, following from my previous comment, I'd probably just define Lock::from_resolution_graph or something. And that might in turn require exposing more stuff from the graph, but maybe that's okay.

(I don't think we've really settled on a great balance here. I wonder, for example, whether it really makes sense to have a ResolutionGraph at all. But this gets to the "installation might want different types" idea that's been floating around. It's a bigger refactor for sure.)

charliermarsh · 2024-06-03T18:38:04Z

I think I will leave the representation as-is for now because it closely mirrors the pyproject.toml schema, where you have an optional-dependencies map that's keyed on extra name. That was intentional, because I'm hoping to make it possible to reify the distribution metadata from the lockfile in the future. It's a very good question though.

charliermarsh force-pushed the charlie/ex branch from c6a8af0 to f6798fc Compare June 1, 2024 18:14

charliermarsh requested a review from BurntSushi June 1, 2024 18:16

charliermarsh marked this pull request as ready for review June 1, 2024 18:16

charliermarsh commented Jun 1, 2024

View reviewed changes

charliermarsh force-pushed the charlie/ex branch from f6798fc to 43a8c09 Compare June 1, 2024 18:20

charliermarsh added the preview Experimental behavior label Jun 1, 2024

charliermarsh requested a review from ibraheemdev June 1, 2024 18:28

ibraheemdev approved these changes Jun 3, 2024

View reviewed changes

BurntSushi approved these changes Jun 3, 2024

View reviewed changes

Omit base package dependencies for extras

54c9c75

charliermarsh force-pushed the charlie/ex branch 3 times, most recently from ec7ee43 to eaa1c91 Compare June 3, 2024 18:53

Add to lock

02f3ead

charliermarsh force-pushed the charlie/ex branch from eaa1c91 to 02f3ead Compare June 3, 2024 18:54

charliermarsh enabled auto-merge (squash) June 3, 2024 18:55

charliermarsh merged commit 10cd6b9 into main Jun 3, 2024
46 checks passed

charliermarsh deleted the charlie/ex branch June 3, 2024 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normalize extras in lockfile #3958

Normalize extras in lockfile #3958

charliermarsh commented Jun 1, 2024 •

edited

Loading

charliermarsh Jun 1, 2024

BurntSushi Jun 3, 2024

BurntSushi Jun 3, 2024

charliermarsh Jun 3, 2024

charliermarsh commented Jun 1, 2024

codspeed-hq bot commented Jun 1, 2024 •

edited

Loading

ibraheemdev left a comment

ibraheemdev Jun 3, 2024

BurntSushi left a comment

BurntSushi Jun 3, 2024

BurntSushi Jun 3, 2024

charliermarsh commented Jun 3, 2024

Normalize extras in lockfile #3958

Normalize extras in lockfile #3958

Conversation

charliermarsh commented Jun 1, 2024 • edited Loading

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliermarsh commented Jun 1, 2024

codspeed-hq bot commented Jun 1, 2024 • edited Loading

Merging #3958 will not alter performance

Summary

ibraheemdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BurntSushi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliermarsh commented Jun 3, 2024

charliermarsh commented Jun 1, 2024 •

edited

Loading

codspeed-hq bot commented Jun 1, 2024 •

edited

Loading