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

python pin now finds workspace root folder when called in subfolder #5035

Closed

Conversation

silvanocerza
Copy link
Contributor

@silvanocerza silvanocerza commented Jul 13, 2024

Summary

Fixes #4971.

This PR changes python pin behaviour when called in a workspace subfolder. Previously it would only read or create a .python-version file in the current working directory, now instead it will first search for the workspace root folder and read or create the .python-version file in that folder.

We assume the root folder contains a valid pyproject.toml file.

If we're not in a workspace use the previous behaviour and creates or reads .python-version in the current working directory.

Test Plan

I added a new test to verify this new behaviour, and run all tests locally.

@silvanocerza silvanocerza force-pushed the python-pin-workspace-subfolder branch 2 times, most recently from cd1a763 to a8452f1 Compare July 13, 2024 09:37
@zanieb
Copy link
Member

zanieb commented Jul 13, 2024

We might want to make this asymmetric per #4971 (comment) and read from the workspace root by default but, when writing, require an opt-in via --workspace otherwise we write to the current directory. Or should we require an opt-out via --isolated to ignore the workspace and write to the working directory? What do you think? I'm not 100% certain what the best UX is yet.

@T-256
Copy link
Contributor

T-256 commented Jul 13, 2024

Or should we use --isolated to ignore the workspace and write to the working directory?

IMO this is more correct to me, since pinned python version is usually used for venv creation/recreation and venv is in workspace root.

@silvanocerza
Copy link
Contributor Author

I would agree with @T-256 here, it makes sense to me using the --isolated option to ignore the workspace.
Should I got with that?

@silvanocerza silvanocerza force-pushed the python-pin-workspace-subfolder branch from a8452f1 to 7c09387 Compare July 14, 2024 15:07
@silvanocerza
Copy link
Contributor Author

Rebased to bring in CI fix from #5034 and remove some commented out code that I missed.

@zanieb zanieb self-assigned this Jul 14, 2024
@zanieb
Copy link
Member

zanieb commented Jul 14, 2024

Sure let's just make --isolated ignore the workspace to start and we can tackle more complex behavior separately?

@silvanocerza
Copy link
Contributor Author

Ready for review again. 👍

Comment on lines -77 to +98
if exists {
writeln!(printer.stdout(), "Replaced existing pin with `{output}`")?;
let mut message = if exists {
format!("Replaced existing pin with `{output}`")
} else {
writeln!(printer.stdout(), "Pinned to `{output}`")?;
}
format!("Pinned to `{output}`")
};

if target_dir != std::env::current_dir()? {
// Print the version file use to pin only
// if it's not in the current working directory
message = format!("{message} in `{}`", version_file.display());
};

writeln!(printer.stdout(), "{message}")?;
Copy link
Contributor Author

@silvanocerza silvanocerza Jul 15, 2024

Choose a reason for hiding this comment

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

I'm not sure about this part, would something like this be better? 🤔

    let message = if exists {
        format!("Replaced existing pin with `{output}`")
    } else {
        format!("Pinned to `{output}`")
    };

    let message = if target_dir != std::env::current_dir()? {
        // Print the version file use to pin only
        // if it's not in the current working directory
        format!("{message} in `{}`", version_file.display())
    } else {
        message
    };

    writeln!(printer.stdout(), "{message}")?;

@zanieb
Copy link
Member

zanieb commented Jul 19, 2024

Going to be lots of conflicts with #5215 and #4989 — I'll own resolving them.

@zanieb
Copy link
Member

zanieb commented Jul 19, 2024

Okay I resolved the conflicts but then realized we need to make a lot of other changes here, e.g. every other command needs to respect pins in the workspace. Kind of complicated, I need to think about it before moving forward.

@zanieb
Copy link
Member

zanieb commented Jul 19, 2024

@silvanocerza
Copy link
Contributor Author

If you can give me a list of the commands that actually needs to respect the pin I should be able to handle that, I'm just unsure about all the commands.

Also probably we need to isolate the logic that finds the pin location, it would simplify implementing it for the other commands.

@zanieb
Copy link
Member

zanieb commented Jul 20, 2024

I think we need to walk up the directory tree looking for a version file like we do with pyproject.toml files for project roots. Then, every command that needs a Python interpreter needs to respect the version file when determining the default. We don't need to do it all at once, I think outside the preview commands only venv supports version files right now and it's gated by preview.

We may or may not want to stop searching at a boundary where we find a pyproject.toml. If we don't, then we need to check if the version is pinned to something incompatible with the current project's Python requirement. If the pin comes from a directory above the project, then we probably need to warn and use the Requires-Python value instead.

It seems simplest to do something like:

  • Add a utility to the version file module that finds the nearest version file. We probably want a struct for this so we can store the path as state and allow read / write.
  • Add a utility to the workspace / project abstractions to reconcile the discovered version file with the project, e.g. we can check if it's location is a prefix of the project or if it's inside the project and check compatibility with the project Python requirement.
  • Use in uv venv under preview mode
  • Use in all the new preview commands where we currently respect version files
  • Add a tracking issue enumerating the remaining commands

How does that sound? Kind of figuring this out as I go :)

@zanieb
Copy link
Member

zanieb commented Sep 16, 2024

I'm taking this on in #6370

@zanieb zanieb closed this Sep 16, 2024
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.

uv python pin should find .python-version file in a workspace
3 participants