-
Notifications
You must be signed in to change notification settings - Fork 582
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
Application of config settings when installing package depends on content of uv cache #7028
Comments
smackesey
added a commit
to dagster-io/dagster
that referenced
this issue
Sep 4, 2024
## Summary & Motivation The `uv` cache can cause subtle problems sometimes when rebuilding pyright environments. Rebuilding envs without using the cache can fix an apparently intractable problem. This PR adds `--no-cache` as an arg to `run-pyright.py`. This is intended for rare use as an escape hatch for pyright env problems-- it makes rebuilding the envs much more slow. See this issue I posted over at `uv` for some info about why you might want to use this: astral-sh/uv#7028 ## How I Tested These Changes My pyright was (unresolvable imports for all dagster packages). After rebuilding my env with `--no-cache` the problem went away. The issue was that the cache seemed to be holding `pth` files for our dagster editables that did not respect `editable_mode=compat` and were incorrectly formatted for pyright. ## Changelog [New | Bug | Docs] NOCHANGELOG
Yeah, I think these definitely aren't included in the cache key. |
I think using |
Thanks, this is working and has enabled a workaround |
👍 I’m still looking into fixing it as part of some other caching improvements. |
smackesey
added a commit
to dagster-io/dagster
that referenced
this issue
Sep 6, 2024
) ## Summary & Motivation This updates `scripts/run-pyright.py` to guard against a very frustrating failure mode in environment buliding. Editable installs place a `.pth` file into a python environment's `site-packages` directory. A "pth" file is resolved by the python import system to some directory that is not already present in `site-packages`. There is a "modern" and "legacy" version of `pth` files. Build backends like `setuptools` (which is what we use for dagster packages) will generate a modern pth file by default during install. The modern pth file contains code that has to be executed, and **is not interpretable by pyright (or any other static static analyzer, to my knowledge)**. The legacy pth file format is simply an absolute path to the target directory. The legacy format is understood by pyright. This means that environments built for pyright must use 100% legacy-style pth files. If you have modern-style pth files in the environment, it will result in mysterious unresolvable import errors, since pyright just skips these files when resolving imports (and unhelpfully does not warn about them). In the run-pyright.py script, we pass `--config-setting editable_mode=compat` when building the python environments, which is supposed to make sure all editables are installed legacy-style. However, the `uv` cache does not account for `--config-setting`, so if an editable install has previously been done for the target package using `uv`, the cached pth may be used and the `editable_mode=compat` setting will be ignored, resulting in the presence of modern pth files in the pyright env and befuddling pyright import errors. This PR does two things to fix this: - Adds `--reinstall-package X` args for all editables to the `uv pip install` command used to build the pyright environment. This makes `uv` skip the cache. - Adds an environment validation step at the end of environment building where we detect any modern-style pth files in the environment and error on them. Note also the uv cache issue is being tracked here: astral-sh/uv#7028 (comment) ## How I Tested These Changes `make rebuild_pyright` after populating the `uv` cache with modern-style dagster pth files. Confirmed that the built environment received only legacy style. ## Changelog [New | Bug | Docs] NOCHANGELOG
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
We use
uv
to build the python environments that we run pyright against for our large codebase Dagster. Pyright environments do not handle Python import hooks correctly. It is therefore important that all editables installs in the environment use a "legacy" style, where the pth file contains a static path instead of an import hook.Modern style:
Legacy style:
The way you force a legacy style when
setuptools
is used as build backend (which is what our packages use) is to passeditable_mode=compat
as a config option. So this should work, and has worked in the past:However, it seems like the cache is interfering in this command working properly. If the package has already been installed as editable without
editable_mode=compat
in one environment, then attempting to install it into a new environment witheditable_mode=compat
does not work.uv version
Repro
Create a toy package
foo
:Create a venv and install foo as an editable:
Notice the content of the above pth is an import hook. That's expected.
But now create another venv and install foo as an editable using
--config-setting editable_mode=compat
. This should cause the content of thepth
file to be a straight path instead of an import hook:It looks like
editable_mode=compat
was ignored because we still have an import hook. Now repeat the above but with--no-cache
:Now it works. So it looks like
--config-setting editable_mode=compat
is being passed through dependent on whether we are using the uv cache.The text was updated successfully, but these errors were encountered: