Skip to content

top: Replace umodule with module everywhere. #9069

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

Merged
merged 21 commits into from
Jun 8, 2023

Conversation

jimmo
Copy link
Member

@jimmo jimmo commented Aug 18, 2022

Follow-up to #9018 -- this is the groundwork for doing the renaming of all built-in modules as well as renaming files and variables. I have always found the "u" prefix confusing, and although weak links provide a workaround there's still weirdness like the output of help('modules') etc. See #9018 for more but I strongly feel we should move in a direction of CPython compatibility and consistency.

Updated from an earlier version of this PR. This is now no longer a breaking change. It continues the decision in #11456 but makes the implementation of that more explicit by allowing modules to be defined as "extensible" or (default) "non-extensible" rather than inferring this from the u-prefix.

In summary:

  • There is now no longer any umodule naming anywhere in the code (file names, qstrs, symbols, macros).
  • A module can now be registered either as MP_REGISTER_MODULE or MP_REGISTER_EXTENSIBLE_MODULE. An extensible module can optionally be overridden from the filesystem, whereas a regular one will always bypass the filesystem. (An extensible module is what would have previously had a u prefix, e.g. uos).
  • In order to retain backwards compatibility, importing an extensible module with it's u prefix will still work (this is kind of the inverse of weak links and is unconditionally enabled), and will bypass the filesystem. New code should prefer to use the sys.path mechanism introduced in py/builtinimport: Allow built-in modules to be packages (v3) #11456 instead (and hopefully we can deprecate the special u-handling later).
  • sys.path is now assignable (using the same "mutable sys" mechansim as sys.ps1 etc).

Reducing the size of the QSTRs has a small code size decrease (~100 bytes), but this is offset by the extra u-prefix handling and the split built-in modules maps, so there's a net negative code size increase (+12 bytes on PYBV11, +84 bytes on bare-arm). We could make the u-prefix handling conditional, but previously the minimal ports only supported "import umodule" so we should keep that working. However, this PR also removes the timeq module which gives a net saving around -850 bytes on PYBV11.

This work was funded through GitHub Sponsors.

@robert-hh
Copy link
Contributor

@jimmo

due to a limitation of the bash scripts I used to automate most of this. I will fix it up of course if we decide to merge.

Looking at the massive number files that have been changed this looks like needing a lot of changes.
Since I keep two ports alive until they get eventually merged (at least one of them), is is possible to provide the bash script you used for changing the sources?

@jimmo jimmo force-pushed the remove-umodule branch 5 times, most recently from af00296 to 7e6a9d1 Compare August 18, 2022 13:57
@jimmo
Copy link
Member Author

jimmo commented Aug 18, 2022

I have updated the tests to translate

try:
  import foo
except:
  import foo

to just import foo.

Since I keep two ports alive until they get eventually merged (at least one of them), is is possible to provide the bash script you used for changing the sources?

@robert-hh ... I did this as a series of (many!) one-liners. There was a bit of trial and error and some manual fixups.

Almost all of the changes are outside the ports, but in terms of rebasing a port, the one-liner below should get you a long way though if you run this on your branch before doing the rebase.

umod.txt

cd ports/samd
# See umod.txt attached to this comment.
cat ~/mpy/umod.txt | while read mod; do git grep -l $mod | xargs sed -i -r "s/(^|[^a-z])$mod($|[^a-z])/\1${mod:1}\2/g"; done
# ignore the no input files messages

@robert-hh
Copy link
Contributor

Thanks. I will try that when the time comes.

@codecov
Copy link

codecov bot commented Jun 1, 2023

Codecov Report

Merging #9069 (a1fbb19) into master (d080d42) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #9069      +/-   ##
==========================================
- Coverage   98.40%   98.40%   -0.01%     
==========================================
  Files         156      155       -1     
  Lines       20622    20539      -83     
==========================================
- Hits        20294    20211      -83     
  Misses        328      328              
Impacted Files Coverage Δ
extmod/modbinascii.c 100.00% <ø> (ø)
extmod/moductypes.c 97.83% <ø> (ø)
extmod/modwebsocket.c 89.76% <ø> (ø)
extmod/vfs_posix.c 93.49% <ø> (ø)
extmod/vfs_posix_file.c 92.22% <ø> (ø)
py/builtin.h 100.00% <ø> (ø)
py/moderrno.c 100.00% <ø> (ø)
py/modio.c 98.52% <ø> (ø)
py/modstruct.c 100.00% <ø> (ø)
py/obj.h 100.00% <ø> (ø)
... and 28 more

@github-actions
Copy link

github-actions bot commented Jun 1, 2023

Code size report:

   bare-arm:   +84 +0.149% 
minimal x86:  +352 +0.189% [incl +32(data)]
   unix x64: -2280 -0.286% standard[incl -416(data) -24(bss)]
      stm32:  -816 -0.208% PYBV10[incl -16(bss)]
        rp2:   -36 -0.011% PICO[incl -12(bss)]

@jimmo jimmo force-pushed the remove-umodule branch 2 times, most recently from 812ca18 to 3d083a1 Compare June 1, 2023 17:30
@jimmo
Copy link
Member Author

jimmo commented Jun 1, 2023

Now that #11456 is merged I have rebased & updated this.

For the time being I have not added a replacement for weak links. We need a way to annotate built-in modules as extensible-from-the-filesystem or not, and then match non-extensible ones at the start of import and extensible at the end. Also a mechanism to allow backwards-compatible import ufoo.

@jimmo jimmo force-pushed the remove-umodule branch from 3d083a1 to 8a17042 Compare June 2, 2023 03:30
@jimmo
Copy link
Member Author

jimmo commented Jun 2, 2023

Added a mechanism to replace the former weak link system and make explicit the behavior defined in #11456 about extensible vs non-extensible modules. Updated the PR description with details.

@jimmo jimmo force-pushed the remove-umodule branch 7 times, most recently from ca40556 to 1bd7804 Compare June 2, 2023 13:56
@jimmo
Copy link
Member Author

jimmo commented Jun 2, 2023

OK all tests passing at last!

@dlech @stinos @andrewleech I think this should alleviate the concerns from #9018. My intention with this PR is now that the only user-visible difference is the output of help("modules") and what you see if you print a module object (i.e. <module 'os'> rather than <module 'uos'>). All existing code should maintain the same behavior.

For people working in the codebase, the main change is the file and symbol renaming, but overall I believe this improves consistency and clarity.

@jimmo jimmo force-pushed the remove-umodule branch from e550aa1 to 677a6d5 Compare June 8, 2023 07:46
@dpgeorge
Copy link
Member

dpgeorge commented Jun 8, 2023

I have finished a full and detailed review of this PR. There are many changes done in the commits here, but a lot of them are simple renames, and most of those are in boilerplate code (eg defining modules and functions).

It's definitely great to see things like the tests simplified to just do import time, instead of the existing try import utime as time; except ImportError: import time.

Overall the changes here are a really good step forward. It simplifies module naming, makes MicroPython behave more like CPython, still allows extensibility of built-ins.

jimmo added 21 commits June 8, 2023 17:53
This renames the builtin-modules, such that help('modules') and printing
the module object will show "module" rather than "umodule".

This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
In order to keep "import umodule" working, the existing mechanism is
replaced with a simple fallback to drop the "u".

This makes importing of built-ins no longer touch the filesystem, which
makes a typical built-in import take ~0.15ms rather than 3-5ms.

(Weak links were added in c14a816)

This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
This work was funded through GitHub Sponsors.

Also updates #includes.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
Updates any includes, and references from Makefiles/CMake.

This essentially reverts what was done long ago in commit
136b5cb

This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
This replaces the previous QSTR_null entry in the globals dict which could
leak out to Python (e.g. via iteration of mod.__dict__) and could lead to
crashes.

It results in smaller code size at the expense of turning a lookup into a
loop, but the list it is looping over likely only contains one or two
elements.

To allow a module to register its custom attr function it can use the new
`MP_REGISTER_MODULE_DELEGATION` macro.

This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
When compiling mpy-cross, there is no `sys` module, and so there will
be no entries in the `mp_builtin_module_delegation_table`.

MSVC doesn't like this, so instead pretend as if the feature isn't
enabled at all.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
Otherwise you can get into the confusing state where e.g. sys.ps1 is
enabled in config (via `MICROPY_PY_SYS_PS1_PS2`) but still doesn't actually
get enabled.

Also verify that the required delegation options are enabled in modsys.c.

This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
Previously sys.path could be modified by append/pop or slice assignment.

This allows `sys.path = [...]`, which can be simpler in many cases, but
also improves CPython compatibility.

It also allows sys.path to be set to a tuple which means that you can
clear sys.path (e.g. temporarily) with no allocations.

This also makes sys.path (and sys.argv for consistency) able to be disabled
via mpconfig. The unix port (and upytesthelper) require them, so they
explicitly verify that they're enabled.

This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
Make tests run in an isolated environment (i.e. `import io` would
otherwise get the `tests/io` directory).

This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
Make tests run in an isolated environment (i.e. `import io` would
otherwise get the `tests/io` directory).

This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
Make tests run in an isolated environment (i.e. `import io` would
otherwise get the `tests/io` directory).

This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
Applies to drivers/examples/extmod/port-modules/tools.

This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
- Update guide for extending built-in modules.
- Remove any last trace of umodule in other docs.

This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
This is a MicroPython-specific module that existed to support the old
version of uasyncio.  It's undocumented and not enabled on all ports and
takes up code size unnecessarily.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
@jimmo jimmo force-pushed the remove-umodule branch from 677a6d5 to a1fbb19 Compare June 8, 2023 07:54
@dpgeorge dpgeorge merged commit a1fbb19 into micropython:master Jun 8, 2023
@dpgeorge
Copy link
Member

dpgeorge commented Jun 8, 2023

Thanks @jimmo for all the hard work on this one.

peterzuger added a commit to peterzuger/hydrogen-micropython that referenced this pull request Jun 26, 2023
for micropython/micropython#9069
handle MICROPY_PY_RANDOM_SEED_INIT_FUNC not being defined
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extmod Relates to extmod/ directory in source ports Relates to multiple ports, or a new/proposed port py-core Relates to py/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants