◐ Shell
reader mode source ↗
Skip to content

gh-69605: Check for already imported modules in PyREPL module completion#139461

Merged
ambv merged 24 commits into
python:mainfrom
loic-simon:pyrepl-module-completion-check-for-already-imported-modules
Jan 5, 2026
Merged

gh-69605: Check for already imported modules in PyREPL module completion#139461
ambv merged 24 commits into
python:mainfrom
loic-simon:pyrepl-module-completion-check-for-already-imported-modules

Conversation

@loic-simon

@loic-simon loic-simon commented Sep 30, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR handles an edge-case when suggesting module imports in PyREPL (see this comment):

If a module is already imported, the import machinery will only get submodules from it, even if a module of the same name is higher in the path. This can happen if eg. sys.path has been updated since the module was originaly imported.

The easiest way to reproduce this issue (and how I stumbled on it) is with stdlib modules imported during interpreter startup (before site customisation, so ignoring potential shadowing):

% mkdir collections
% touch collections/__init__.py collections/foo.py
% python
Python 3.14.0rc2 [...]
>>> from collections import <TAB>
>>> from collections import foo
Traceback (most recent call last):
  File "<python-input-0>", line 1, in <module>
    from collections import foo
ImportError: cannot import name 'foo' from 'collections' (/Users/loic/cpython/Lib/collections/__init__.py)
>>>

This PR add a special case in _pyrepl.ModuleCompleter._find_modules that

  • check if the module to complete imports from is already imported
  • if yes, search for imports from the imported module location only, instead of using the global cache

It was a little tricky to get right, I'm not sure about everything 😅 but it pass all tests I could think off!

cc @tomasr8

@bedevere-app bedevere-app Bot mentioned this pull request Sep 30, 2025
Closed
@loic-simon loic-simon mentioned this pull request Sep 30, 2025
Merged
@loic-simon

loic-simon commented Sep 30, 2025

Copy link
Copy Markdown
Contributor Author

Ok, one of the new test fails on Windows x64/32 (but pass on arm)

I just setup and built Python on a Win 11 x64 to try to reproduce, the test pass 😵‍💫 (and the fix works)
If anyone has an idea of what may interfere? (Python 3.15.0a0 (main, Sep 30 2025, 23:36:53) [MSC v.1944 64 bit (AMD64)] on win32)

@python-cla-bot

python-cla-bot Bot commented Oct 1, 2025 •
edited
Loading

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed

@picnixz picnixz marked this pull request as draft October 3, 2025 13:21
@picnixz

picnixz commented Oct 3, 2025

Copy link
Copy Markdown
Member

I'm going to convert it into a draft until you deem this PR ready. That will also reduce the probability of someone clicking on the PR to review it (like me...) while it's not yet ready for.

@loic-simon

loic-simon commented Oct 3, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I'm going to convert it into a draft until you deem this PR ready. That will also reduce the probability of someone clicking on the PR to review it (like me...) while it's not yet ready for.

Thanks, I just fixed the failing test, this should be ready for review!

Sorry again for the noise, I was dealing with what turned out to be a importer cache issue, which I couldn't reproduce outside of the CI buildbots 😓

[edit] I just noticed I can convert to draft / mark ready myself, will do it next times!

@loic-simon loic-simon marked this pull request as ready for review October 5, 2025 13:38
10 hidden items Load more…
@tomasr8 tomasr8 self-requested a review December 28, 2025 15:29
loic-simon
loic-simon commented Jan 1, 2026
View reviewed changes

@loic-simon loic-simon left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hide comment

I also realized we will need the same logic for my other PR about attributes completion, so I factored the logic into a private method, I suggest we merge this one first so I can rebase merge main into the other and call it!

EDIT: ok that's not true, since attributes completion checks directly the module object, I forgot 😅 but the refactor is a good thing anyway I think

@loic-simon loic-simon requested a review from tomasr8 January 1, 2026 18:14
Hide details View details @ambv ambv merged commit bfac54d into python:main Jan 5, 2026
55 checks passed
@ambv

ambv commented Jan 5, 2026

Copy link
Copy Markdown
Contributor

This is good, confirmed on macOS and Windows it's doing the right thing!

@bedevere-bot

bedevere-bot commented Jan 5, 2026

Copy link
Copy Markdown

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot s390x RHEL9 LTO + PGO 3.x (tier-3) has failed when building commit bfac54d.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1578/builds/3864) and take a look at the build logs.
  4. Check if the failure is related to this commit (bfac54d) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/1578/builds/3864

Failed tests:

  • test_pyrepl

Failed subtests:

  • test_already_imported_module_without_origin_or_spec - test.test_pyrepl.test_pyrepl.TestPyReplModuleCompleter.test_already_imported_module_without_origin_or_spec

Summary of the results of the build (if available):

==

Click to see traceback logs 
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.lto-pgo/build/Lib/test/test_pyrepl/test_pyrepl.py", line 1195, in test_already_imported_module_without_origin_or_spec
    module = importlib.import_module(mod)
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.lto-pgo/build/Lib/importlib/__init__.py", line 88, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap>", line 1303, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1276, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1240, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'no_spec'


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.lto-pgo/build/Lib/test/test_pyrepl/test_pyrepl.py", line 1195, in test_already_imported_module_without_origin_or_spec
    module = importlib.import_module(mod)
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.lto-pgo/build/Lib/importlib/__init__.py", line 88, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap>", line 1303, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1276, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1240, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'not_has_location'

@ambv

ambv commented Jan 5, 2026

Copy link
Copy Markdown
Contributor

The buildbot failure is related. I'm on it.

@bedevere-bot

bedevere-bot commented Jan 5, 2026

Copy link
Copy Markdown

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot s390x RHEL9 LTO 3.x (tier-3) has failed when building commit bfac54d.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1587/builds/3876) and take a look at the build logs.
  4. Check if the failure is related to this commit (bfac54d) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/1587/builds/3876

Failed tests:

  • test_pyrepl

Failed subtests:

  • test_already_imported_module_without_origin_or_spec - test.test_pyrepl.test_pyrepl.TestPyReplModuleCompleter.test_already_imported_module_without_origin_or_spec

Summary of the results of the build (if available):

==

Click to see traceback logs 
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.lto/build/Lib/test/test_pyrepl/test_pyrepl.py", line 1195, in test_already_imported_module_without_origin_or_spec
    module = importlib.import_module(mod)
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.lto/build/Lib/importlib/__init__.py", line 88, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap>", line 1303, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1276, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1240, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'not_has_location'


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.lto/build/Lib/test/test_pyrepl/test_pyrepl.py", line 1195, in test_already_imported_module_without_origin_or_spec
    module = importlib.import_module(mod)
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.lto/build/Lib/importlib/__init__.py", line 88, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap>", line 1303, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1276, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1240, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'no_spec'

@ambv

ambv commented Jan 5, 2026

Copy link
Copy Markdown
Contributor

I suspect this is about file system timestamp resolution. Lemme attempt a fix.

@tomasr8

tomasr8 commented Jan 5, 2026

Copy link
Copy Markdown
Member

@ambv Shall we backport this PR (& and the buildbot fix)? I think this can be considered a bugfix.

ambv pushed a commit to ambv/cpython that referenced this pull request Jan 5, 2026
@loic-simon @tomasr8 @ambv
pythongh-69605: Check for already imported modules in PyREPL module c… …
7db209b 
…ompletion (pythonGH-139461)

Co-authored-by: Tomas R. <tomas.roun8@gmail.com>
@ambv

ambv commented Jan 5, 2026

Copy link
Copy Markdown
Contributor

Yup, gh-143438.

thunder-coding pushed a commit to thunder-coding/cpython that referenced this pull request Feb 15, 2026
@loic-simon @tomasr8 @thunder-coding
pythongh-69605: Check for already imported modules in PyREPL module c… …
55f5b8b 
…ompletion (pythonGH-139461)

Co-authored-by: Tomas R. <tomas.roun8@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tomasr8 tomasr8 tomasr8 left review comments

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

@lysnikolaou lysnikolaou lysnikolaou is a code owner

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

+1 more reviewer

@ashm-dev ashm-dev ashm-dev requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@loic-simon @picnixz @ambv @bedevere-bot @tomasr8 @ashm-dev @nazeerali4325-commits