Skip to content

Change the definition of --locked to require satisfaction check#6102

Merged
charliermarsh merged 1 commit intomainfrom
charlie/locked
Aug 15, 2024
Merged

Change the definition of --locked to require satisfaction check#6102
charliermarsh merged 1 commit intomainfrom
charlie/locked

Conversation

@charliermarsh
Copy link
Member

Summary

This PR changes the definition of --locked from:

Produces the same Lock

To:

Passes Lock::satisfies

This is a subtle but important difference. Previous, if Lock::satisfies failed, we would run a resolution, then do existing_lock == lock. If the two weren't equal, and --locked was specified, we'd throw an error.

The equality check is hard to get right. For example, it means that we can't ship #6076 without changing our marker representation, since the deserialized lockfile "loses" some of the internal marker state that gets accumulated during resolution.

The downside of this change is that there could be scenarios in which uv lock --locked fails even though the lockfile would actually work and the exact TOML would be unchanged. But... I think it's ok if --locked fails after the user modifies something?

@charliermarsh charliermarsh marked this pull request as ready for review August 15, 2024 01:24
@charliermarsh charliermarsh added cli Related to the command line interface preview Experimental behavior labels Aug 15, 2024
@charliermarsh
Copy link
Member Author

I'm now trying to think of a situation in which the user could change something (like a workspace requirement), and Lock::satisfies fails, but then the TOML is totally unchanged. Is that even possible anymore?

@charliermarsh
Copy link
Member Author

Anyway, I think this is another change that would reduce a lot of complexity for us while being basically the same for users.

@ibraheemdev
Copy link
Member

The downside of this change is that there could be scenarios in which uv lock --locked fails even though the lockfile would actually work and the exact TOML would be unchanged.

I'm not sure I understand this, shouldn't this change allow strictly more lockfiles to satisfy --locked?

@charliermarsh
Copy link
Member Author

I'm not sure I understand this, shouldn't this change allow strictly more lockfiles to satisfy --locked?

I don't think so, because before, if you failed Lock::satisfies, we'd still try to resolve, then compare the contents of the lockfiles. Now, if you fail Lock::satisfies, we'll error without trying to resolve. I think the new condition is stricter.

@charliermarsh
Copy link
Member Author

Does that make sense?

@ibraheemdev
Copy link
Member

I see, that makes sense. So now we're verifying against the requirements, not the new lockfile.

@charliermarsh
Copy link
Member Author

Yeah, that's a fair description

@ibraheemdev
Copy link
Member

I'm not sure how this compares to changing the markers to not store the range restriction state. Also trying to think of an example where the requirements change but a resolution would have resulted in the same lockfile (so --locked would now fail where it previously succeeded). I guess if you relaxed a version range for a package that is also depended on transitively, so the locked version does not change.. but with requires-dist now I guess that also changes the lockfile?

@charliermarsh
Copy link
Member Author

I guess if you relaxed a version range for a package that is also depended on transitively, so the locked version does not change.. but with requires-dist now I guess that also changes the lockfile?

Yeah this is where I'm ending up too.

Copy link
Member

@ibraheemdev ibraheemdev left a comment

Choose a reason for hiding this comment

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

I think the part about requires-dist making the lockfile change where it previously wouldn't is a little unfortunate, but if we decide to do that (separate from this PR) then this makes sense to me.

@charliermarsh
Copy link
Member Author

I think the part about requires-dist making the lockfile change where it previously wouldn't is a little unfortunate, but if we decide to do that (separate from this PR) then this makes sense to me.

I'm wondering if this is really that big of a problem. If we don't go through Lock::satisfies, we still use the lockfile as preferences and the user may even have a warm cache. It's probably still super fast 🤔

@charliermarsh
Copy link
Member Author

Maybe just to show my hand a bit: I think this fast path is critical for uv run, where the vast majority of invocations won't have experienced any lockfile changes, and we want as little overhead as we can get there. Everywhere else, I view it as more of a nice-to-have...

@charliermarsh
Copy link
Member Author

Maybe one thing for me to think through tomorrow -- what happens when:

  • User clones a repo, adds a dependency to the pyproject.toml and runs uv lock.
  • User clones a repo, runs uv sync, then later adds a dependency to the pyproject.toml and runs uv lock.

What do we reuse? Where are we being inefficient?

@ibraheemdev
Copy link
Member

The other thing we lost is user clones a repo and runs uv lock with a cold cache (again somewhat unrelated to this change).

@charliermarsh
Copy link
Member Author

I think that’s unchanged, no? What’s different there?

@ibraheemdev
Copy link
Member

ibraheemdev commented Aug 15, 2024

Sorry, you're right, that hasn't changed. Adding a dependency is where it changed.

@charliermarsh
Copy link
Member Author

Yeah, adding a dependency (that was already in the lockfile, I think? But not explicit) changed.

@ibraheemdev
Copy link
Member

Any resolution that didn't require fetching versions of existing packages that were not in the lockfile changed. For example, adding a new dependency that did not affect the locked versions of any existing dependencies could use the prefilled index from the lockfile and only fetch the metadata for the new package.

Not sure how big of an impact that optimization had though, and I think we can still make it in the future.

Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

I think we should do this because it moves uv to a more conservative posture. But I do think we should improve on this in the future. Some specific thoughts:

The equality check is hard to get right. For example, it means that we can't ship #6076 without changing our marker representation, since the deserialized lockfile "loses" some of the internal marker state that gets accumulated during resolution.

Is this something that could also be fixed by a custom equality comparison?

The downside of this change is that there could be scenarios in which uv lock --locked fails even though the lockfile would actually work and the exact TOML would be unchanged. But... I think it's ok if --locked fails after the user modifies something?

I agree. Or at least, I think this is the right decision to make right now.

I'm now trying to think of a situation in which the user could change something (like a workspace requirement), and Lock::satisfies fails, but then the TOML is totally unchanged. Is that even possible anymore?

One example is:

  • In pyproject.toml, there is a dependency foo>=1.0.
  • In uv.lock, foo is locked to 1.2.
  • I make a change to pyproject.toml by updating the dependency on foo to foo>=1.1.

This is a somewhat strained example, but it's definitely something I've done a few times in the Rust world where I realized my minimum version constraint was wrong (because I was depending on features in foo released in 1.1). So it's not like it never happens. But I think this is a case where Lock::satisfies will fail, but a re-resolve (assuming nothing in the world changed) should produce the same semantic uv.lock. (The actual TOML would be different because we write out the full requirements, and that changed, but the actual locked version that we install wouldn't change.)

But like, I don't necessarily think that is a major issue. Or at least, not a release blocking issue. Unless there are other more important use cases that I'm missing. That is, I think the worst thing that happens here is that folks might need to re-run uv lock in some cases where they might not otherwise have to.

@charliermarsh
Copy link
Member Author

👍 I agree with the above. (And yeah, the TOML would change in that format, but the locked dependencies would not.)

@charliermarsh charliermarsh merged commit 6333823 into main Aug 15, 2024
@charliermarsh charliermarsh deleted the charlie/locked branch August 15, 2024 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cli Related to the command line interface preview Experimental behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants