You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Revisiting "checking a module in the environment it is imported from"
#5531
Revisiting "checking a module in the environment it is imported from"
Background
Pyrefly keys all per-module state on Handle. Today a Handle is a module name, a path, and a SysInfo (Python version and platform). If two imports agree on these three values, Pyrefly solves the module once and shares the result.
In file mode (pyrefly check <files>) and in the language server, each file gets the config of the folder it is in. That config includes the site-package path, so the file's imports resolve in its own folder's .venv.
This does not match Python at runtime. An interpreter has one sys.path. When app/.venv/bin/python imports a library, the library's imports resolve in app/.venv, wherever the library source is. So one library file can resolve to different modules, depending on which interpreter imports it. We've been OK with this to simplify the design. See Jia's comment here for details.
Where's the problem?
Projects that have more than one venv, where code from one venv is installed into another. The usual cases are editable installs of a sibling checkout, and monorepos with one venv per package (pip or Poetry). Bug report: "DataFrame is not assignable to DataFrame" From #3556:
app and lib each have a config and a venv. mylib is installed in editable mode into app/.venv, so app imports the source in lib/ directly.
When you run pyrefly check app/main.py:
main.py uses app/'s config, so import pandas resolves to app/.venv/.../pandas.
import mylib follows the editable install to lib/mylib/. Pyrefly checks lib/mylib/frames.py with the config of its own folder, lib/.
So frames.py's import pandas resolves to lib/.venv/.../pandas, a second copy.
Pyrefly now has two DataFrame classes and reports:
ERROR Argument `DataFrame` is not assignable to parameter `df` with type `DataFrame` in function `mylib.frames.width` [bad-argument-type]
At runtime only step 1's copy exists. mypy and pyright report no errors on the same code. The error message prints the same type on both sides, so users cannot see the cause.
Project mode does not have this problem, because one config covers the whole run.
Proposal 1
Add an optional environment to Handle (D128474304, based on PR #5414 by Yasser Sheikh):
Handle gets an optional SitePackagePath field, an interned list of paths.
An imported module gets the importer's site-package path only when both of these are true:
the import was found under the importer's site-package path, and
the module's own config has a different site-package path.
A module gets a second copy only when it is reached from a second environment. With the venv inside the project, third-party modules stay one module. With a venv outside the project, opening a library file directly (for example, through go-to-definition) can create a second copy.
When a Handle has a site-package path, State::get_config returns a cached copy of the module's config with that path. The module's imports then resolve to the same modules as the importer's imports.
Open files keep their own folder's environment. A library file that you open in the IDE is checked as it is today.
pyright, basedpyright, and ty use the same model, with the workspace folder as the unit. Drawbacks:
Modules can be checked more than once (benchmarks don't show any immediate regressions)
Mypy Primer does not do a good job of testing this, since it is mostly relevant with editable installs and multiple environments, while mypy primer runs everything under single-environment project-based checks.
Proposal 2: Limit to editable paths
We could limit proposal 1 to interpreter_editable_path and avoid other site packages. This might fix most of the issues while limiting the scope. This would avoid cases where the site-package path comes from the pyrefly config instead of the interpreter:
reacted with thumbs up emoji reacted with thumbs down emoji reacted with laugh emoji reacted with hooray emoji reacted with confused emoji reacted with heart emoji reacted with rocket emoji reacted with eyes emoji
Uh oh!
There was an error while loading. Please reload this page.
Revisiting "checking a module in the environment it is imported from"
Background
Pyrefly keys all per-module state on
Handle. Today aHandleis a module name, a path, and aSysInfo(Python version and platform). If two imports agree on these three values, Pyrefly solves the module once and shares the result.In file mode (
pyrefly check <files>) and in the language server, each file gets the config of the folder it is in. That config includes the site-package path, so the file's imports resolve in its own folder's.venv.This does not match Python at runtime. An interpreter has one
sys.path. Whenapp/.venv/bin/pythonimports a library, the library's imports resolve inapp/.venv, wherever the library source is. So one library file can resolve to different modules, depending on which interpreter imports it. We've been OK with this to simplify the design. See Jia's comment here for details.Where's the problem?
Projects that have more than one venv, where code from one venv is installed into another. The usual cases are editable installs of a sibling checkout, and monorepos with one venv per package (pip or Poetry).
Bug report: "
DataFrameis not assignable toDataFrame" From #3556:appandlibeach have a config and a venv.mylibis installed in editable mode intoapp/.venv, soappimports the source inlib/directly.When you run
pyrefly check app/main.py:main.pyusesapp/'s config, soimport pandasresolves toapp/.venv/.../pandas.import mylibfollows the editable install tolib/mylib/. Pyrefly checkslib/mylib/frames.pywith the config of its own folder,lib/.frames.py'simport pandasresolves tolib/.venv/.../pandas, a second copy.Pyrefly now has two
DataFrameclasses and reports:At runtime only step 1's copy exists. mypy and pyright report no errors on the same code. The error message prints the same type on both sides, so users cannot see the cause.
Project mode does not have this problem, because one config covers the whole run.
Proposal 1
Add an optional environment to
Handle(D128474304, based on PR #5414 by Yasser Sheikh):Handlegets an optionalSitePackagePathfield, an interned list of paths.Handlehas a site-package path,State::get_configreturns a cached copy of the module's config with that path. The module's imports then resolve to the same modules as the importer's imports.pyright, basedpyright, and ty use the same model, with the workspace folder as the unit.
Drawbacks:
Proposal 2: Limit to editable paths
We could limit proposal 1 to
interpreter_editable_pathand avoid other site packages. This might fix most of the issues while limiting the scope. This would avoid cases where the site-package path comes from the pyrefly config instead of the interpreter:Monorepos with
PYTHONPATHset,conda develop ../libpython setup.py developand handmade.pthwould not apply.Proposal 3: more project-based checks
If we opt for proposal 3 here, file-based checks will become less common. Is this enough to avoid solving this issue?
All reactions