bpo-45324: Capture data in FrozenImporter.find_spec() to use in exec_module(). #28633

ericsnowcurrently · 2021-09-29T18:18:00Z

Currently we end up duplicating effort and throwing away data in FrozenImporter.find_spec(). With this change we do the work once in find_spec() and the only thing we do in FrozenImporter.exec_module() is turn the raw frozen data into a code object and then exec it.

The change involves adding _imp.find_frozen(), adding an arg to _imp.get_frozen_object(), and updating FrozenImporter. We also move some code around to reduce some duplication, get a little more consistency in outcomes, and be more efficient.

Note that this change is mostly necessary if we want to set __file__ on frozen stdlib modules. (See https://bugs.python.org/issue21736.)

https://bugs.python.org/issue45324

bedevere-bot · 2021-10-01T01:40:45Z

🤖 New build scheduled with the buildbot fleet by @ericsnowcurrently for commit c6e70cc 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

bedevere-bot · 2021-10-01T03:18:53Z

🤖 New build scheduled with the buildbot fleet by @ericsnowcurrently for commit c6e70cc 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

brettcannon

Some of my questions are probably more, "add a comment" or "rename to be clearer". 😄

brettcannon · 2021-10-01T19:43:59Z

Lib/importlib/_bootstrap.py

+        spec = spec_from_loader(fullname, cls,
+                                origin=cls._ORIGIN,
+                                is_package=ispkg)
+        spec.loader_state = (data,)


Why the tuple? It isn't future-proofing anything as you will still have to change any other accesses unless you index into this (which you don't; see line 860). Otherwise I dictionary could also be used if you're trying to avoid issues w/ the future needing more than one piece of data.

But honestly, I would just keep it simple for now and directly assign to the attribute.

brettcannon · 2021-10-01T19:45:46Z

Lib/importlib/_bootstrap.py

+        name = spec.name
+        try:
+            data, = spec.loader_state
+        except Exception:


This is extremely broad for just reading an attribute. What are you specifically guarding against? If you're worried about the attribute not being set can't you just check for None?

Hmm, I thought I'd changed that to AttributeError. I'll fix it.

brettcannon · 2021-10-01T19:48:13Z

Lib/importlib/_bootstrap.py

+                                  name=name)
+            data = None
+        else:
+            # We clear the extra data we got from the finder, to save memory.


So the loader is one-time use? What happens if you call exec_module() again (e.g. importlib.reload())? Won't this break that?

It should work fine. If the data is set then the _imp.get_frozen_object() call in exec_module() will unmarshal that. If the data isn't set (e.g. with a reload) then get_frozen_object() will look up the frozen data, in addition to unmarshaling it.

I've added to that comment to clarify.

brettcannon · 2021-10-01T19:51:32Z

Python/import.c

+    FROZEN_BAD_NAME,    // The given module name wasn't valid.
+    FROZEN_NOT_FOUND,   // It wasn't in PyImport_FrozenModules.
+    FROZEN_DISABLED,    // -X frozen_modules=off (and not essential)
+    FROZEN_EXCLUDED,    // The PyImport_FrozenModules entry has NULL "code".


Does this mean the search should continue somewhere else? When is this used to block the import search somewhere else?

I'm not sure what the original motivation of that status is. It pre-dates this PR. I only consolidated the case into the enum.

I don't mind investigating but it falls outside the scope of this PR and it would be hard to justify prioritizing it otherwise.

https://bugs.python.org/issue45379

brettcannon · 2021-10-01T19:52:41Z

Python/import.c

+    FROZEN_NOT_FOUND,   // It wasn't in PyImport_FrozenModules.
+    FROZEN_DISABLED,    // -X frozen_modules=off (and not essential)
+    FROZEN_EXCLUDED,    // The PyImport_FrozenModules entry has NULL "code".
+    FROZEN_INVALID,     // The PyImport_FrozenModules entry is bogus.


What does "invalid" mean? Do you mean "corrupt"? I'm just trying to think of a user who sees this and wonder how they might be able to resolve it.

It means someone manually put invalid data into PyImport_FrozenModules (or that Tools/scripts/freeze_modules.py is broken). We were already treating this case as FROZEN_NOT_FOUND. I only teased out a separate enum member to make the condition more clear.

I don't mind adding more notes in the comment but to me the best thing to do is to look at the places where that status is emitted.

I don't mind adding more notes in the comment but to me the best thing to do is to look at the places where that status is emitted.

Do you mean in set_frozen_error()?

I was referring to the enum value rather than the exception. In the case of FROZEN_INVALID, that would be in find_frozen() where we return FROZEN_INVALID (or likewise unmarshal_frozen_code() or _imp.get_frozen_object()).

https://bugs.python.org/issue45379

brettcannon · 2021-10-01T19:54:38Z

Python/import.c

    if (name == NULL) {
        PyErr_Clear();
-        return NULL;
+        return FROZEN_BAD_NAME;


Does PyUnicode_AsUTF8() not set an exception? That seems like something that should propagated up instead of silenced.

It does set an exception, hence PyErr_Clear(). I added that in gh-28320, where PyUnicode_AsUTF8() replaced the existing usage of _PyUnicode_EqualToASCIIString(). The latter either asserts in error cases or ignores the errors, so I followed suit, effectively matching the existing behavior of the code. I don't mind propagating the error but didn't want to change behavior.

I've added a note.

brettcannon · 2021-10-01T19:56:35Z

Python/import.c

    }
+
    if (!use_frozen() && !is_essential_frozen_module(name)) {


What does use_frozen() represent? The use of && suggests that function represents use_essential_frozen_modules() would be a better name.

use_frozen() says whether or not frozen modules should be used. (See the -X frozen_modules CLI option.) We call is_essential_module() to make sure we still use the necessary frozen modules. If you are suggesting that I combine the two functions, note that I did so in that original PR, @zooba suggested I not, and I agreed. 🙂

My point is I thought -X frozen_modules is for built-in frozen modules, not all frozen modules, and so use_frozen() doesn't seemed labelled appropriately based on that understanding.

Ah, thanks for clarifying. What do you mean by "built-in"? I wasn't aware of different kinds of frozen modules, other than that a few of them must be used no matter what for importlib bootstrap.

Right, but what this also means is if I use frozen modules in my project and I for some reason use -X frozen_modules then you just broke me. That's probably fine, but I was personally viewing all of this on/off work as only affecting what was in the stdlib, not the entire frozen module mechanism (except for the "essential" modules which are technically just the import-related ones). That's why the names were throwing me.

Okay, that makes sense. So should we restrict the flag to stdlib modules (-X frozen_stdlib)? If so then yeah, the function should be named "use_frozen_stdlib".

I don't know what's best, but at least we now understand why I kept getting confused. 😅

Cool. We can deal with "-X frozen_stdlib" separately.

FYI, I'm going to change the -X option to "frozen_stdlib" (and the import.c function to "use_frozen_stdlib"). Thanks for pointing this out.

brettcannon · 2021-10-01T20:02:56Z

Python/import.c

-                     name);
-        return NULL;
+    if (info != NULL) {
+        info->nameobj = nameobj;  // borrowed


Suggested change

info->nameobj = nameobj; // borrowed

info->nameobj = nameobj; // Borrowed.

It isn't a sentence and I'd rather not make it one. I'd rather drop the comment then add noise.

The comment itself can't hurt but yeah, it doesn't need to be sentence-ized.

brettcannon · 2021-10-01T21:52:31Z

Lib/test/test_importlib/frozen/test_finder.py

+        self.assertIsNotNone(spec.loader_state)
+
+    def check_search_location(self, spec, source=None):
+        # For now frozen packages do not have any path entries.


Suggested change

# For now frozen packages do not have any path entries.

# Frozen packages do not have any path entries.

bedevere-bot · 2021-10-01T21:55:48Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

ericsnowcurrently · 2021-10-02T01:40:36Z

I have made the requested changes; please review again.

bedevere-bot · 2021-10-02T01:40:39Z

Thanks for making the requested changes!

@brettcannon: please review the changes made to this pull request.

Lib/importlib/_bootstrap.py

Python/import.c

ericsnowcurrently requested review from brettcannon, encukou, ncoghlan and warsaw as code owners September 29, 2021 18:18

the-knights-who-say-ni added the CLA signed label Sep 29, 2021

bedevere-bot added the awaiting core review label Sep 29, 2021

ericsnowcurrently mentioned this pull request Sep 30, 2021

bpo-45020: Identify which frozen modules are actually aliases. #28655

Merged

ericsnowcurrently added 15 commits September 30, 2021 18:42

Make use of find_frozen() more consistent.

97bdaa2

Drop some one-off helper functions.

e2e676d

Factor out unmarshal_frozen_code().

6333212

Add a "data" arg to _imp.get_frozen_object().

8cd239b

Add _imp.find_frozen().

7724019

Use _imp.find_frozen() in the frozen importer.

0dd3870

Clear loader_state to save memory.

1a0ad16

Only call is_frozen() in the fallback case.

fdf7689

Add a NEWS entry.

385d412

Add a missing "static" declaration.

e0fe501

Treat FROZEN_BAD_NAME as FROZEN_NOT_FOUND in some cases.

7221e0e

Make frozen_info.size Py_ssize_t.

5f68ee8

Py_ssize_t is never smaller than int.

7b85a63

name -> nameobj.

cdbc611

Add tests.

c6e70cc

ericsnowcurrently force-pushed the frozen-modules-find-spec branch from f7c009e to c6e70cc Compare October 1, 2021 00:43

ericsnowcurrently added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 1, 2021

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 1, 2021

ericsnowcurrently added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 1, 2021

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 1, 2021

ericsnowcurrently force-pushed the frozen-modules-find-spec branch from 0c94188 to 8b8cf51 Compare October 1, 2021 18:21

ericsnowcurrently added 2 commits October 1, 2021 12:44

Tweak the tests.

f86dcd5

Default to using _imp.get_frozen_object().

d1b1868

ericsnowcurrently force-pushed the frozen-modules-find-spec branch from 8b8cf51 to d1b1868 Compare October 1, 2021 18:46

ericsnowcurrently added 2 commits October 1, 2021 13:44

Do not use <> to mark source package.

4e105a9

Drop unused code.

5d6af1e

brettcannon requested changes Oct 1, 2021

View reviewed changes

bedevere-bot removed the awaiting core review label Oct 1, 2021

bedevere-bot added the awaiting changes label Oct 1, 2021

ericsnowcurrently added 6 commits October 1, 2021 18:53

Store data in loader_state directly.

017c932

Set "name" on import errors.

1a833ed

Use a narrower exception.

5363708

Fix a comment.

07c947f

Clarify a comment.

cddf089

Add a note about why we silence an error.

ca0500f

bedevere-bot added awaiting change review and removed awaiting changes labels Oct 2, 2021

bedevere-bot requested a review from brettcannon October 2, 2021 01:40

warsaw reviewed Oct 3, 2021

View reviewed changes

Lib/importlib/_bootstrap.py Show resolved Hide resolved

warsaw reviewed Oct 4, 2021

View reviewed changes

Python/import.c Show resolved Hide resolved

ericsnowcurrently mentioned this pull request Oct 5, 2021

Freeze/link all stdlib modules imported during startup. faster-cpython/ideas#82

Closed

46 tasks

ericsnowcurrently merged commit c3d9ac8 into python:main Oct 5, 2021

ericsnowcurrently deleted the frozen-modules-find-spec branch October 5, 2021 16:01

bedevere-bot removed the awaiting change review label Oct 5, 2021

This was referenced Oct 5, 2021

bpo-45324: Rearrange some of the FrozenImporter finder tests. #28740

Merged

bpo-45396: Always import custom frozen modules. #28776

Closed

ericsnowcurrently mentioned this pull request Oct 28, 2021

gh-89559: Change -X frozen_modules to -X frozen_stdlib. #29301

Closed

	info->nameobj = nameobj; // borrowed
	info->nameobj = nameobj; // Borrowed.

	# For now frozen packages do not have any path entries.
	# Frozen packages do not have any path entries.

bpo-45324: Capture data in FrozenImporter.find_spec() to use in exec_module(). #28633

bpo-45324: Capture data in FrozenImporter.find_spec() to use in exec_module(). #28633

Conversation

ericsnowcurrently commented Sep 29, 2021 • edited by bedevere-bot Loading

bedevere-bot commented Oct 1, 2021

bedevere-bot commented Oct 1, 2021

brettcannon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericsnowcurrently Oct 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Oct 1, 2021

ericsnowcurrently commented Oct 2, 2021

bedevere-bot commented Oct 2, 2021

ericsnowcurrently commented Sep 29, 2021 •

edited by bedevere-bot

Loading

ericsnowcurrently Oct 2, 2021 •

edited

Loading