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

Retain and respect settings in tool upgrades #5937

Merged
merged 2 commits into from
Aug 9, 2024

Conversation

charliermarsh
Copy link
Member

Summary

We now persist the ResolverInstallerOptions when writing out a tool receipt. When upgrading, we grab the saved options, and merge with the command-line arguments and user-level filesystem settings (CLI > receipt > filesystem).

@charliermarsh charliermarsh added enhancement New feature or improvement to existing functionality preview Experimental behavior labels Aug 8, 2024
`flask>=3` is already installed
"###);

// It should update the receipt though.
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know, I guess? So the receipt represents the "most recent" settings.

Copy link
Member

Choose a reason for hiding this comment

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

fwiw for the lockfile we only write non-default values

Copy link
Member

Choose a reason for hiding this comment

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

I think updating here makes sense.

"###);
});

// Reinstall with `highest`. This is a no-op, since we _do_ have a compatible version installed.
Copy link
Member Author

Choose a reason for hiding this comment

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

Hard to say what the right behavior is here. We could also consider discarding the tool whenever settings differ.

Copy link
Member

Choose a reason for hiding this comment

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

This is really tough. It seems okay to change after some user feedback. I'm leaning towards consistent --upgrade / --reinstall semantics where it is required to change the versions. This matches uv pip install, yes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah

Copy link
Member Author

Choose a reason for hiding this comment

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

Now that we have upgrade, I'm sort of open to making install more aggressive about replacing though.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense too.

Copy link
Member Author

Choose a reason for hiding this comment

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

So we could make it replace whenever the settings or versions differ...

/// The options persisted alongside an installed tool.
///
/// A mirror of [`ResolverInstallerOptions`], without upgrades and reinstalls, which shouldn't be
/// persisted in a tool receipt.
Copy link
Member Author

Choose a reason for hiding this comment

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

We go through some effort to avoid adding --upgrade to tool receipts. Does that seem right? Otherwise, uv tool install black --upgrade saves upgrade in the receipt...

Copy link
Member

Choose a reason for hiding this comment

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

Yes that would be weird.

`flask>=3` is already installed
"###);

// It should update the receipt though.
Copy link
Member

Choose a reason for hiding this comment

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

fwiw for the lockfile we only write non-default values

@zanieb
Copy link
Member

zanieb commented Aug 9, 2024

I will review this morning.

@charliermarsh
Copy link
Member Author

Would most appreciate review of the correct / desired behaviors.

Comment on lines +2465 to +2470
let executable = bin_dir.child(format!("flask{}", std::env::consts::EXE_SUFFIX));
assert!(executable.exists());

// On Windows, we can't snapshot an executable file.
#[cfg(not(windows))]
Copy link
Member

Choose a reason for hiding this comment

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

Aside: We should probably open an issue to define a macro for this, since it's non-trivial and repeated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah good idea. I can do it.

@zanieb
Copy link
Member

zanieb commented Aug 9, 2024

Sorry got half way through this then had meetings and forgot — reading now.

@charliermarsh charliermarsh merged commit f89403f into main Aug 9, 2024
56 checks passed
@charliermarsh charliermarsh deleted the charlie/index-receipt branch August 9, 2024 18:21
Copy link

codspeed-hq bot commented Aug 9, 2024

CodSpeed Performance Report

Merging #5937 will degrade performances by 11.18%

Comparing charlie/index-receipt (4cf3560) with main (4df0fe9)

Summary

❌ 2 regressions
✅ 12 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main charlie/index-receipt Change
wheelname_tag_compatibility[flyte-long-incompatible] 1.4 µs 1.6 µs -11.15%
wheelname_tag_compatibility[flyte-short-incompatible] 926.9 ns 1,043.6 ns -11.18%

zanieb pushed a commit that referenced this pull request Aug 9, 2024
We now persist the `ResolverInstallerOptions` when writing out a tool
receipt. When upgrading, we grab the saved options, and merge with the
command-line arguments and user-level filesystem settings (CLI > receipt
> filesystem).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement to existing functionality preview Experimental behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants