[ty] Infer the Python version from the environment if feasible #18057
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
zanieb
commented
May 12, 2025
zanieb
commented
May 13, 2025
zanieb
force-pushed
the
zb/python-env-version
branch
from
9632346 to
72ff26a
Compare
|
zanieb
force-pushed
the
zb/python-env-version
branch
from
72ff26a to
79e2d99
Compare
|
A second opinion seems helpful. Not in a rush to merge before the announcement though. |
zanieb
requested review from
carljm,
AlexWaygood,
sharkdp,
dcreager and
MichaReiser
as code owners
MichaReiser
reviewed
May 13, 2025
MichaReiser
reviewed
May 13, 2025
There was a problem hiding this comment.
We should do some more testing for how this interacts in --watch mode, and, ideally, at some tests to file_watching.rs.
Two cases that I'm not sure if they work as expected:
- Changing the settings from having a
python-versionto not having one (in which case the fallback behavior should apply) - Changing the python version in the venv. I don't think ty would pick this up and it would keep running with the wrong version
|
Thanks! Those are good callouts. I'll follow up on this when I have time, otherwise, feel free. |
|
This would be great to have (especially since I accidentally said at the typing summit that we already had it 😆 ) |
|
I'll try to pick this up this week |
|
IMO, not sure if it's worth prioritizing this over other typing features. |
|
I disagree:
The better we do at inferring the Python version from the user's environment, the less likely it is we have to fallback to an arbitrary default Python version, which is always going to lead to false positives and false negatives for some users. Improving usability in this area feels pretty high priority to me. |
AlexWaygood
force-pushed
the
zb/python-env-version
branch
2 times, most recently
from
4a3c360 to
d7b6fdb
Compare
|
Here's a screen recording that demonstrates that we correctly update the inferred Python version in Screen.Recording.2025-05-29.at.18.55.55.movI checked, and we also get the correct behaviour if you delete a pyproject.toml file that contains a |
|
Okay, I think this is ready for re-review now! In terms of tests, I added two CLI tests and recorded the video in #18057 (comment) to demonstrate that the file-watching behaviour is working. We have fairly comprehensive tests already for |
|
(Cc. @zanieb -- would love a review from you of the changes I've pushed here 😄) |
|
Ah, the primer report is interesting. I guess an unanticipated consequence of this change is that we're now picking up the Python version from the virtual environments mypy_primer creates on the fly if primer is being run on a project that doesn't specify |
AlexWaygood
reviewed
May 29, 2025
Comment on lines
385
to
395
| /// A parser for `pyvenv.cfg` files: metadata files for virtual environments. | ||
| /// | ||
| /// Note that a `pyvenv.cfg` file *looks* like a `.ini` file, but actually isn't valid `.ini` syntax! | ||
| /// | ||
| /// See also: <https://snarky.ca/how-virtual-environments-work/> | ||
| #[derive(Debug)] | ||
| struct PyvenvCfgParser<'s> { | ||
| cursor: Cursor<'s>, | ||
| line_number: NonZeroUsize, | ||
| data: RawPyvenvCfg<'s>, | ||
| } |
There was a problem hiding this comment.
The switch to a proper parser here is motivated by wanting to capture the TextRange of the Python version in the pyvenv.cfg file so that we can display it in diagnostics. Screenshot of what this looks like on the command line:
Can you say more about what you need and what you tried? |
I wanted to add a test that showed that if you added a virtual environment, ty would switch to using the Python version from that virtual environment if no Python version was specified in any configuration files. I tried something like this: diff --git a/crates/ty/tests/file_watching.rs b/crates/ty/tests/file_watching.rs
index 71c50ff6eb..d8a398f276 100644
--- a/crates/ty/tests/file_watching.rs
+++ b/crates/ty/tests/file_watching.rs
@@ -1170,6 +1170,42 @@ print(sys.last_exc, os.getegid())
Ok(())
}
+#[test]
+fn change_python_version_due_to_new_venv() -> anyhow::Result<()> {
+ let mut case = setup(|context: &mut SetupContext| {
+ // `sys.last_exc` is a Python 3.12 only feature;
+ // we default to the latest Python version.
+ context.write_project_file("bar.py", "import sys; print(sys.last_exc)")
+ })?;
+
+ let diagnostics = case.db.check();
+
+ assert!(diagnostics.is_empty());
+
+ let venv_dir = case.project_path(".venv").as_std_path().to_path_buf();
+ std::fs::create_dir_all(&venv_dir)?;
+ std::fs::write(venv_dir.join("pyvenv.cfg"), "version = 3.8")?;
+
+ // Register the venv as a search path:
+ // we should now pick up the Python version from the venv.
+ case.update_options(Options {
+ environment: Some(EnvironmentOptions {
+ extra_paths: Some(vec![RelativePathBuf::cli(".venv/site-packages")]),
+ ..EnvironmentOptions::default()
+ }),
+ ..Options::default()
+ })?;
+
+ let diagnostics = case.db.check();
+ assert_eq!(diagnostics.len(), 1);
+ assert_eq!(
+ diagnostics[0].primary_message(),
+ "Type `<module 'sys'>` has no attribute `last_exc`"
+ );
+
+ Ok(())
+}
+but it did not pass, even though manual testing appears to indicate that file watching is working as intended. (LMK if that's not the case!) |
This is so cool! File watching tests always have the following structure:
Your code misses the steps 3 and 4. |
AlexWaygood
force-pushed
the
zb/python-env-version
branch
from
21bacec to
84f151a
Compare
AlexWaygood
force-pushed
the
zb/python-env-version
branch
from
84f151a to
33c6941
Compare
|
I'm still struggling to write a file-watching test. I'm probably missing something, but I'm just going to land for now. Happy to follow up on this if there's an easy way to get it working! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Posting the draft to preview the change here and decide if it's fine or if we need to do a more invasive refactor.
Before falling back to the default Python version, use the Python version in the target Python environment.
Only supports reading the version from the
pyvenv.cfgright now. We can sniff target versions from the site packages folder name on Unix too? A bit messy though.