FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix caching behavior of PEP561-installed namespace packages by msullivan · Pull Request #10937 · python/mypy · GitHub

Repository navigation

Fix caching behavior of PEP561-installed namespace packages - #10937

Merged
hauntsaninja merged 2 commits into
masterfrom
flip-flop
Aug 6, 2021
Merged

hauntsaninja merged 2 commits into
masterfrom
flip-flop

Conversation

Copy link
Copy Markdown
Collaborator

Since PEP561-installed namespace packages only show up in
FindModuleCache as a side-effect, the submodules need to be listed
first in the dependencies for things to work. This was handled already
in the Import case but not the ImportFrom case.

Also fix the cache handling of namespace packages so it doesn't always
get reported stale.

Fixes #9852.

msullivan commented Aug 6, 2021 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

CI is failing because of the toml nonsense

hauntsaninja left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yay!!! Thank you for figuring this out!!

I really somehow did not see the very helpful comment on L738 (the Import case) despite looking right at this code the other day. Also hopeful this PR fixes some of the issues #9636 was running into!

Comment thread mypy/build.py Outdated
source = ''
self.source = source
if path and source is None and self.manager.cache_enabled:
if path and self.manager.cache_enabled:

hauntsaninja Aug 6, 2021 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Could you explain what's going on here? Naively I'd expect a change on L1844 (since it would mirror the change in parse_file)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yeah, the idea was that we still need to parse the cache even when it is a namespace package (because it hit that isdir check just above). I just changed it to restore the old check, but sank the isdir stuff futher down

Comment thread mypy/build.py
# priority.
pri = import_priority(imp, PRI_HIGH if not all_are_submodules else PRI_LOW)
res.insert(pos, ((pri, cur_id, imp.line)))
# The imported module goes in after the

hauntsaninja Aug 6, 2021 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

(I was curious why this was an res.insert to start with, since insert is nobody's first choice. It looks like it was introduced in the dawn of incremental mode #1292 and was previously a res.append before the loop. It was moved after the loop to change the priority, and so I guess res.insert was to maintain the previous position)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ah, interesting!

Copy link
Copy Markdown
Collaborator

(Lol, just saw your discord comments match my PR comments)

hauntsaninja reopened this Aug 6, 2021
Since PEP561-installed namespace packages only show up in
FindModuleCache as a side-effect, the submodules need to be listed
first in the dependencies for things to work. This was handled already
in the Import case but not the ImportFrom case.

Also fix the cache handling of namespace packages so it doesn't always
get reported stale.

Fixes #9852.
hauntsaninja merged commit c90026b into master Aug 6, 2021
hauntsaninja deleted the flip-flop branch August 6, 2021 20:01

Copy link
Copy Markdown
Collaborator

Hoorah!

hauntsaninja added a commit to hauntsaninja/mypy that referenced this pull request Apr 11, 2025
Fixes python#12664

A root cause is there is this stateful `_update_ns_ancestors` thing in
`modulefinder`, so if things get called in the wrong order, you can get
incorrect results.

See also the logic in `all_imported_modules_in_file` where we've fixed
several bugs like this previously, like python#13124 and python#10937

As a result of (seemingly accidentally) reusing imports across modules,
we can end up in a situation where the namespace gets added as a
dependency to other modules and so on the cached run we attempt to find
namespace before package, which does not work

I am not sure this `imports` code path is even needed, so I will open an
alternate PR.

I can't write a good test for this because it requires something in
site_packages, but here's a minimal repro:
```
set -eux
rm -rf repro
mkdir repro
cd repro

SITEPACK=env/site-packages
mkdir -p $SITEPACK

mkdir $SITEPACK/ruamel
mkdir $SITEPACK/ruamel/yaml

printf 'from ruamel.yaml.main import *' > $SITEPACK/ruamel/yaml/__init__.py
printf 'import ruamel.yaml' > $SITEPACK/ruamel/yaml/main.py
printf '' > $SITEPACK/ruamel/yaml/py.typed

printf 'import ruamel.yaml' > a.py
printf 'import a' > main.py

rm -rf .mypy_cache
PYTHONPATH=$SITEPACK mypy main.py
PYTHONPATH=$SITEPACK mypy main.py
```
hauntsaninja added a commit that referenced this pull request Apr 11, 2025
Fixes #12664

A root cause is there is this stateful `_update_ns_ancestors` thing in
`modulefinder`, so if things get called in the wrong order, you can get
incorrect results.

See also the logic in `all_imported_modules_in_file` where we've fixed
several bugs like this previously, like #13124 and #10937

As a result of (seemingly accidentally) reusing imports across modules,
we can end up in a situation where the namespace gets added as a
dependency to all other modules and so on the cached run we attempt to
find namespace before package, which does not work

I am not sure this `imports` code path is even needed, so I will open an
alternate PR, see #18908.

Relevant history:
- #6582
- #6179

I can't write a good test for this because it requires something in
site_packages, but here's a minimal repro:
```
set -eux
rm -rf repro
mkdir repro
cd repro

SITEPACK=env/site-packages
mkdir -p $SITEPACK

mkdir $SITEPACK/ruamel
mkdir $SITEPACK/ruamel/yaml

printf 'from ruamel.yaml.main import *' > $SITEPACK/ruamel/yaml/__init__.py
printf 'import ruamel.yaml' > $SITEPACK/ruamel/yaml/main.py
printf '' > $SITEPACK/ruamel/yaml/py.typed

printf 'import ruamel.yaml' > a.py
printf 'import a' > main.py

rm -rf .mypy_cache
PYTHONPATH=$SITEPACK mypy main.py
PYTHONPATH=$SITEPACK mypy main.py
```
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cache causes mypy to fail every 2nd run for module importing from aws_cdk

2 participants


Back | FazBrowse Home | New Git URL