Skip to content

[ty] Search PYTHONPATH to find modules #20441

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 20, 2025
Merged

[ty] Search PYTHONPATH to find modules #20441

merged 3 commits into from
Sep 20, 2025

Conversation

@mmlb
Copy link
Contributor

@mmlb mmlb commented Sep 16, 2025

This brings ty in line with other type checkers and python interpreters, including on by default and no options to disable. Can be disabled by clearing the PYTHONPATH environment variable.

Summary

Prepend PYTHONPATH to extra_paths so ty behaves similar to other type checkers and the interpreters. Fixes astral-sh/ty#547

Test Plan

I built ty locally and ran it in nix-shell environment that sets PYTHONPATH, without this PR ty errors with unresolved-attribute.

@mmlb
Copy link
Contributor Author

mmlb commented Sep 16, 2025

This is modification of #20016, I dropped the namespace package logic and opt-in as requested. Instead of opt-out via flag I went with opt-out by clearing PYTHONPATH as I think that makes sense but would be happy to add an explicit cli arg. Thanks to @natelust for the initial code/PR.

@mmlb mmlb mentioned this pull request Sep 16, 2025
Closed
@mmlb mmlb changed the title Teach TY to search PYTHONPATH to find modules [ty] Search PYTHONPATH to find modules Sep 16, 2025
@ntBre ntBre added the ty Multi-file analysis & type inference label Sep 16, 2025
carljm
carljm reviewed Sep 16, 2025
View reviewed changes
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me. Not sure if it's feasible to test this, or if we should have a CLI flag to disable reading PYTHONPATH (just unsetting PYTHONPATH does seem like a reasonable alternative.). Would prefer for @MichaReiser or @AlexWaygood to review.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 16, 2025 •
edited
Loading

Diagnostic diff on typing conformance tests

No changes detected when running ty on typing conformance tests ✅

@github-actions
Copy link
Contributor

github-actions bot commented Sep 16, 2025 •
edited
Loading

mypy_primer results

No ecosystem changes detected ✅
No memory usage changes detected ✅

@natelust
Copy link
Contributor

natelust commented Sep 16, 2025

Thanks for picking this up, I just have been underwater dealing with things lately, I really appreciate it.

MichaReiser
MichaReiser requested changes Sep 17, 2025
View reviewed changes
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Thank you. I left a few comments. We should also add a CLI test for this in

ruff/crates/ty/tests/cli/python_environment.rs

Line 9 in 1d2128f

fn config_override_python_version() -> anyhow::Result<()> {

crates/ty_project/src/metadata/options.rs
.map(|path| path.absolute(project_root, system)),
);

let settings = SearchPathSettings {
Copy link
Member

@MichaReiser MichaReiser Sep 17, 2025

Choose a reason for hiding this comment

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

@AlexWaygood any thoughts on whether we should move this logic into SearchPaths::from_settings?

Copy link
Member

@AlexWaygood AlexWaygood Sep 17, 2025

Choose a reason for hiding this comment

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

No strong opinion really!

@MichaReiser MichaReiser added the configuration Related to settings and configuration label Sep 17, 2025
@sharkdp sharkdp removed their request for review September 17, 2025 10:21
@mmlb mmlb force-pushed the push-tyvxlwmmrvxu branch 2 times, most recently from 62138e0 to 89f57d1 Compare September 17, 2025 14:16
@mmlb
Copy link
Contributor Author

mmlb commented Sep 17, 2025

Thank you. I left a few comments. We should also add a CLI test for this in

ruff/crates/ty/tests/cli/python_environment.rs

Line 9 in 1d2128f

fn config_override_python_version() -> anyhow::Result<()> {

I'll take a look at this and give it a go.

@mmlb mmlb force-pushed the push-tyvxlwmmrvxu branch from 89f57d1 to 1870de5 Compare September 17, 2025 15:40
MichaReiser
MichaReiser reviewed Sep 17, 2025
View reviewed changes
@mmlb mmlb force-pushed the push-tyvxlwmmrvxu branch from 1870de5 to 46bc82e Compare September 17, 2025 16:24
@MichaReiser
Copy link
Member

MichaReiser commented Sep 18, 2025

Thank you. This looks good now and we can merge it once there's a CLI test.

@AlexWaygood AlexWaygood mentioned this pull request Sep 18, 2025
Closed
@natelust @mmlb @MichaReiser
[ty]: Search PYTHONPATH to find modules
b9f05f7 
This brings ty in line with other type checkers and python interpreters,
including on by default and no options to disable. Can be disabled by
clearing the PYTHONPATH environment variable.

Co-authored-by: Manuel Mendez <[email protected]>
Co-authored-by: Micha Reiser <[email protected]>
@mmlb mmlb force-pushed the push-tyvxlwmmrvxu branch from 46bc82e to b9f05f7 Compare September 19, 2025 21:27
@mmlb
Copy link
Contributor Author

mmlb commented Sep 19, 2025

Thank you. This looks good now and we can merge it once there's a CLI test.

Done!

@mmlb mmlb requested a review from MichaReiser September 19, 2025 21:28
MichaReiser
MichaReiser approved these changes Sep 20, 2025
View reviewed changes
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Perfect, thank you

@MichaReiser MichaReiser merged commit 2c6c3e7 into astral-sh:main Sep 20, 2025
39 checks passed
@MichaReiser MichaReiser mentioned this pull request Sep 20, 2025
Open
@mmlb mmlb deleted the push-tyvxlwmmrvxu branch September 22, 2025 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carljm carljm carljm left review comments

@AlexWaygood AlexWaygood AlexWaygood left review comments

@MichaReiser MichaReiser MichaReiser approved these changes

@dcreager dcreager Awaiting requested review from dcreager dcreager is a code owner

Assignees

No one assigned

Labels

configuration Related to settings and configuration ty Multi-file analysis & type inference

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ty not using PYTHONPATH and PYTHONSAFEPATH

6 participants

@mmlb @natelust @MichaReiser @carljm @AlexWaygood @ntBre