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
Implementation for PEP 451 (importlib.machinery.ModuleSpec) #63064
Comments
This ticket will track the implementation for PEP-451 (ModuleSpec). I'll have a patch up in the next couple days. |
Here's an initial patch. Some key things left to do:
Once that's squared away there are further things that will be addressed in new tickets (or at least separate patches). This includes:
|
Here's a patch just for the changes to the import page in the language reference. It's not perfect, but should represent the target pretty closely. |
Let's try that again with the proper diff. :) |
Thanks, that looks pretty good - so much less work for loaders to do. After this, we may even be able to finally fix the circular import bug and the one where submodule imports succeed but the overall package import fails. |
I've added a review |
Something else we may be able to fix (albeit probably not in 3.4): several of the issues noted in PEP-395 (since __main__.__spec__.name will give the real module name, even when __name__ is set to "__main__" or otherwise modified) |
Here's an update of the language reference patch based on Brett's comments. |
Here's an updated patch that implements the meat of the current PEP. Docs are still lacking and I could probably add a few more tests. Other key things left to do:
|
This time against tip. :) I should also point out that this patch is on top of the path in bpo-19413. |
Here's a new patch that is mostly up to date with the PEP. I still need to work on Docs and add more tests. There are a few failing tests (due to the recent reload patch) that I need to fix when I get a minute. This patch also implements find_spec() on FileFinder and PathFinder. Left to do:
|
I've created a server-side clone for the implementation: |
Okay, I've updated the PEP-451 branch in the clone to include as much of the implementation as I've completed, which is the bulk of the functional changes. It's enough to pass the test suite. Here's what I'd like to get done before the feature freeze (in this priority order):
Other things that can (but don't have to) wait until after the beta release:
I haven't had a chance yet to make any changes to Doc/reference/import.rst in response to Brett's review, but I did make the Doc/library/importlib.rst changes he recommended. |
I don't quite know what you mean by "current functional changes in the clone". Is this changes to import to use exec_module() and such? Just a guess since step 3 suggests there are no loaders implementing the new API currently. As for the tests, I'm hoping that simply refactoring some of them will allow reusing a bunch of the finder & loader tests instead of having to do it from scratch. |
I should clarify why I want a clarification on step 1. On Friday my current plan (barring other bugs I need to squash for b1) is to just start working on the pep-451 repo and I want a TODO list to follow in this issue so I don't have to waste time trying to figure out what to work on next. That's why I want it spelled out if we want the loaders fixed first or import, etc. |
Sorry that wasn't more clear. I committed the changes from the modulespec-primary-changes.diff patch to the PEP-451 branch in the server-side clone. Those changes are: Step 1
Step 2
Step 3
Step 3
Step 5
Others
At this point, the test suite passes and the fundamental changes of the PEP are implemented (on the server-side clone). Here's what's left to do before the feature freeze:
Other things that can (but don't have to) wait until after the beta release:
|
"1." was intentionally left blank <wink> |
Regarding tests, a bunch of importlib (and other?) tests make direct calls to find_module(), find_loader(), or load_module(). These are still working, as expected. However, we should probably either replace them or supplement them with equivalent tests that make use of specs directly. |
Yeah, don't replace any tests, add new ones for the new APIs. Given the A more conservative approach also gives us a chance to make sure we have |
That's what makes the most sense to me too.
I'm fine with that. I seem to remember one place where an actual deprecation warning would be good, but I'll have to double-check.
Nice analogy. "can_load_into()" makes sense. Do you think it's important enough to include in 3.4? I'm not sure it is, since I'd expect it to be a pretty uncommon case that can be worked around pretty easily (since the finder is fully aware of the loader it's creating). The extra loader method would help with that boilerplate operation, but... As we found out (and you expounded) there are a variety of reload/load-into cases that we could address more explicitly. Perhaps there's a better API that could address those needs more broadly, or maybe they're just not worth addressing specifically. Exploring all this is something that can wait, IMHO. |
Although, a boolean query method would bring back the problem of the loader That would address my concern about the lack of useful error information in Another thing we need to check we have a test for: ensuring reloading a This would all be so much easier if reloading wasn't supported in the first |
On 12 Nov 2013 09:36, "Eric Snow" <report@bugs.python.org> wrote:
Yes, that's an option, too, and probably a good one. To go down that path, |
Passing the target to the finders isn't just for the sake of any implicit "check_target" test, though that was the original motivator. It also allows the finder to decide between multiple loaders based on other criteria related to the target (but not necessarily the loader). I think it was a good addition to the API regardless.
Agreed. Furthermore, such a test is worthwhile outside the context of PEP-451. I'm tempted to say we're already covered with existing tests, but reload is goofy enough that an explicit test is worth it.
So very true. :) |
And coming full circle: there's no *harm* in letting finders reject loading That means the only thing we need to postpone is the load_module |
Sounds good. It will be worth adding a note to the load_module() docs indicating the limited cases where it is still appropriate, and encouraging the use of exec_module() instead. |
Semantic-affecting changes first, then fluffy bits in terms of priority. Basically go based on what is most critical to be in 3.4. And yes, basically can you comment out find_module/find_loader/load_module and execute the test suite (sans import-related tests) without import-related failures. |
No longer blocking b1, now just blocking rc1. |
Your commit doesn't compile on Windows. Category None Implement PEP-451 (ModuleSpec). |
There is a commented-out line in Lib/importlib/_bootstrap.py: +# if hasattr(loader, 'get_data'): |
Removed the commented-out line in c8a84eed9155 |
Failing buildbot: http://buildbot.python.org/all/builders/x86%20XP-4%203.x/builds/9630/steps/test/logs/stdio test.test_module.ModuleTests.test_module_repr_source |
The test_module failure can be triggered by running test_importlib test_module. For some reason unittest's __spec__ gets set to one for the BuiltinImporter (and nothing else). |
New changeset 98bab4a03120 by Brett Cannon in branch 'default': |
I just realized that there is no way to make ModuleSpec.has_location return True if the spec is created by simply instantiating ModuleSpec. As it stands now you **must** go through importlib.util.spec_from_file_location(), which makes it not a helper but a **required** API to use. I think this is the only thing that cannot be stated directly through ModuleSpec instantiation or public attribute assignment. This means either (a) the docs on ModuleSpec need to be **very** clear that you must use spec_from_file_location() to make spec.has_location be true, (b) add another keyword-only argument to ModuleSpec.__init__(), or (c) make has_location settable (e.g. be able to only set it to True/False or alternatively assigning it sets both has_location to True and origin to the assigned value). |
Good catch. My preference would be for (c) simply add a setter for the property that sets the underlying private variable to True or False. |
(c) it is then! I would just take the time to call bool() on the arg to coalesce it into a True/False thing (which maybe makes sense in __init__() as well). |
Actually, ignore the __init__() part of my last comment since it's set to False directly. |
Here's a patch for adding a setter for ModuleSpec.has_location. |
LGTM |
New changeset e961a166dc70 by Eric Snow in branch 'default': |
About the only thing left for this ticket is to finish up writing a few tests and round out some documentation. |
There are a few lingering to-do comments that will need to be addressed as part of the remaining work: Lib/test/test_importlib/extension/test_case_sensitivity.py:# XXX find_spec tests |
Failure with 3.4.0c1: [170/394/5] test_importlib
test test_importlib failed -- Traceback (most recent call last):
File "C:\Programs\Python34.32\lib\unittest\case.py", line 57, in testPartExecutor
yield
File "C:\Programs\Python34.32\lib\unittest\case.py", line 574, in run
testMethod()
File "C:\Programs\Python34.32\lib\unittest\loader.py", line 32, in testFailure
raise exception
ImportError: Failed to import test module: test.test_importlib.source.test_abc_loader
Traceback (most recent call last):
File "C:\Programs\Python34.32\lib\unittest\loader.py", line 312, in _find_tests
module = self._get_module_from_name(name)
File "C:\Programs\Python34.32\lib\unittest\loader.py", line 290, in _get_module_from_name
__import__(name)
File "C:\Programs\Python34.32\lib\test\test_importlib\source\test_abc_loader.py", line 73, in <module>
class PyLoaderMock(abc.PyLoader):
AttributeError: 'module' object has no attribute 'PyLoader' On the other hand, the test passes on a fresh debug build. |
Terry has admitted in other bugs he filed that he didn't use a fresh install location, so the test failure is most likely a red herring. |
I've gone ahead and moved all remaining work into separate issues. Once the 4 remaining sub-issues are resolved, this one can be closed. I've *finally* got a couple days free so I want to close this out. :) |
Given that PEP-451 is listed in the "Finished PEPs (done, implemented in code repository)" section of the PEP index can we close this and associated issues out? |
Dependencies 19711 and 21099 are still open. |
The last dependencies have now been closed! |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: