Skip to content

py/builtinimport: support relative import in custom __import__ callbacks #6665

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
Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

py/builtinimport: support relative import in custom __import__ callbacks #6665

wants to merge 4 commits into from
+43 −7

Conversation

@zsquareplusc
Copy link
Contributor

@zsquareplusc zsquareplusc commented Nov 30, 2020

The globals need to be forwarded from the callers context.

This pull request adds a test with relative import and override as well as a fix.

I've tried to use a custom python function for import to make some performance measurements (actually tracking approximate memory usage). There i noticed that's impossible for a python handler function to support relative imports (level > 0). This is due to the globals that mp_builtin___import__ is getting are from the custom callback instead of the module where import/__import__ was called. So the lookup of __path__ gives the wrong value.

The solution is that mp_import_name in runtime.c gets the globals and passes them along in de parameter that already exists for this purpose (i see CPython is doing this too). mp_builtin___import__ uses the globals parameter or falls back to the old behavior if None is passed or fewer positional args are provided.

@hoihu
Copy link
Contributor

hoihu commented Nov 30, 2020

+1

@dpgeorge dpgeorge added the py-core Relates to py/ directory in source label Nov 29, 2021
tannewt added a commit to tannewt/circuitpython that referenced this pull request Jul 30, 2022
@tannewt
Merge pull request micropython#6665 from dhalbert/fix-macos-mpy-cross…
3612ce2 
…-name

fix name for macOS mpy-cross universal build
@Gadgetoid Gadgetoid mentioned this pull request Feb 29, 2024
Merged
@projectgus

This comment was marked as outdated.

Sign in to view
@dpgeorge
Copy link
Member

dpgeorge commented Oct 16, 2025

@zsquareplusc sorry no one took a look at this when it was opened.

It actually looks really good, it fixes a missing piece of the import machinery. Would you be able to rebase this on the latest master? If not, I can do that.

@dpgeorge dpgeorge added this to the release-1.27.0 milestone Oct 17, 2025
@dpgeorge dpgeorge force-pushed the fix-override-relative-import branch from cc31c03 to 2d50d42 Compare November 4, 2025 02:47
@dpgeorge
Copy link
Member

dpgeorge commented Nov 4, 2025

I have rebased this on latest master. It was a bit tricky due to the many merge commits, but I managed to retain the original patch.

@github-actions
Copy link

github-actions bot commented Nov 4, 2025 •
edited
Loading

Code size report:

Reference:  tests/serial_test.py: Allow up to 2 seconds between bytes. [2762fe6]
Comparison: tests/import: Split out type test, remove .exp. [merge of bd758b8]
  mpy-cross:    +0 +0.000% 
   bare-arm:   +12 +0.021% 
minimal x86:   +47 +0.025% 
   unix x64:   +72 +0.008% standard
      stm32:   +36 +0.009% PYBV10
     mimxrt:   +40 +0.011% TEENSY40
        rp2:   +48 +0.005% RPI_PICO_W
       samd:   +44 +0.016% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:   +48 +0.011% VIRT_RV32

@codecov
Copy link

codecov bot commented Nov 4, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.38%. Comparing base (2762fe6) to head (bd758b8).

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #6665   +/-   ##
=======================================
  Coverage   98.38%   98.38%           
=======================================
  Files         171      171           
  Lines       22294    22298    +4     
=======================================
+ Hits        21933    21937    +4     
  Misses        361      361

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

zsquareplusc and others added 4 commits November 6, 2025 13:07
@zsquareplusc @dpgeorge
py/builtinimport: support relative import in custom __import__ callbacks
56391c0 
The globals need to be forwarded from the callers context.
@zsquareplusc @dpgeorge
extmod/asyncio: Support for overriden __import__.
308705d 
globals() needs to be provided in case __import__ is a Python function
@dpgeorge
py/builtinimport: Only parse globals if level>0.
f8d170b 
Following CPython behaviour.

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge
tests/import: Split out type test, remove .exp.
bd758b8 
Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge dpgeorge force-pushed the fix-override-relative-import branch from 2d50d42 to bd758b8 Compare November 6, 2025 02:07
@dpgeorge
Copy link
Member

dpgeorge commented Nov 6, 2025 •
edited
Loading

I adjusted the code here so it better matches CPython (only check the globals argument if level>0). And removed the error message to reduce code size (this error message telling that globals must be a dict would rarely be hit in the real world, so not worth spending bytes on it).

Also adjusted the tests:

  • removed the .py.exp file for the test, now that the output matches CPython
  • moved parts of the test to an existing test file, they have a better home there

@oliver-joos
Copy link
Contributor

oliver-joos commented Nov 12, 2025

Only 2 weeks ago I prepared a rebased branch of this PR. However I encountered open questions. And my goal was just making custom __import__ work despite relative imports, not CPython compatibility.

Now I compared my branch with this PR. This PR has:

  • less code for setting globals in mp_builtin___import___default()
  • TypeError check moved from tests/import/import_override2.py to builtin_import.py
  • no irrelevant renaming of test symbols like on my branch

My branch is well tested since years. And the differences of this PR look good to me.
I am happy if this PR gets merged!

@dpgeorge
Copy link
Member

dpgeorge commented Nov 13, 2025

Thanks @oliver-joos for the feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

py-core Relates to py/ directory in source

Projects

None yet

Milestone

release-1.27.0

Development

Successfully merging this pull request may close these issues.

5 participants

@zsquareplusc @hoihu @projectgus @dpgeorge @oliver-joos