[red-knot] Discover local venv folder in cli

[MatthewMckee4]

Summary

Fixes astral-sh/ty#171

Code from
https://github.com/astral-sh/uv/blob/bbf4f830b5b346d9bdba18b6bc319c9527e5e92c/crates/uv-python/src/virtualenv.rs#L124-L144

Test Plan

Manual testing

[github-actions]

mypy_primer results

No ecosystem changes detected ✅

[MichaReiser]

I'm not sure if implementing the fallback here, at least how it's done now is the most ideal solution. I've two concerns:

1. This re-implements part of the discovery logic in VirtualEnvironment::new
2. It's now possible that Red Knot picks up a .venv for which VirtualEnvironment::new later fails (for example, because the pyenv.cfg is invalid). Red Knot will fail to start

I think my preferred behavior is that Red Knot ignores any errors (may log a warning or debug message) if a .venv directory exists but it isn't a valid venv. But I don't think Red Knot should fail to start in that case because it isn't an input explicitly set by the user.

One way to implement this is to have a PythonPath::Discover option where the fallback behavior is implemented in SearchPaths::from_settings

[MatthewMckee4]

It's now possible that Red Knot picks up a .venv for which VirtualEnvironment::new later fails (for example, because the pyenv.cfg is invalid). Red Knot will fail to start

Could this not happen already? It could pick up the env variable of an invalid .venv right?

[MatthewMckee4]

Could we say we just don't error for site packages discovery for these?

SysPrefixPathOrigin::VirtualEnvVar | SysPrefixPathOrigin::LocalVenv

[MichaReiser]

Could this not happen already? It could pick up the env variable of an invalid .venv right?

IMO, this is different because the user explicitly sets the VIRTUAL_ENV environment variable. They're explicitly telling Red Knot to use that folder.

Could we say we just don't error for site packages discovery for these?

We could do that. But I wonder if it still leads to some more code duplication compared to deferring more to the resolver.

[MatthewMckee4]

Do you think its still bad to look for a venv there

                .or_else(PythonPath::from_local_venv)

Then in SearchPaths::from_settings, we allow an invalid venv, and fallback to no site packages?

[MatthewMckee4]

Are you thinking of reverting to what we had before discovery of any venv:

            python_path: python
                .map(|python_path| {
                    PythonPath::SysPrefix(python_path.absolute(project_root, system))
                })
                .unwrap_or_else(|| PythonPath::KnownSitePackages(vec![])),

Then do all discovery of venvs inside SearchPathSettings, or in SearchPaths.from_settings?

[MichaReiser]

No, I'd introduce a new PythonPath variant because the resolver otherwise can't know that it should do the venv discovery (KnownSitePackages would then become testing only)

[MatthewMckee4]

Okay I think understand, I'll have a go at this

[MatthewMckee4]

How does that look?

[MatthewMckee4]

Sorry yeah now KnownSitePackages is only used in tests.

What is KnownSitePackages currently used for anyway, if its only used with an empty vec. Seems there is no use right now other than a fallback

[MichaReiser]

Sorry, for being slow to respond. We're having an on-site and have very limited time on GitHub. I'll come back to your question no later than next week; I just don't know when. Sorry for that.

[MatthewMckee4]

All good, no worries at all, thank you.

[MichaReiser]

I'm interested into @AlexWaygood's opinion but I don't think we should test if the current (or any parent) directory is a venv. It's unclear to me why you'd want to run red knot inside a venv.

Let's also add some debug logs when we pick up a venv (here or in the ::Discover match arm so that users understand that Red Knot picked up a venv.

[MatthewMckee4]

Okay sure, i just took this straight from uv, i can clean it up.

Ill add some debug logs first

[MatthewMckee4]

Also, isn't checking if your in a venv pointless. If you are in a venv, then you will eventually get to the parent of the venv which will detect the venv. I'll remove this