Importlib #1018
Importlib #1018
Conversation
Maybe |
|
This importlib stuff is pretty complex, thank you for having a look at this. The change looks pretty okay to me. It looks like pretty entangled code of frozen, import and the vm. Does the |
I would be my preference to initialize this optionally, seperate from the vm, so that we could re-use the vm without importlib if this is required. So in this case, I would suggest to initialize importlib in |
|
Why would you like to make this optional? For the WASM we can just replace the |
|
I would like to make this optional for cases where you do not want to to use importlib. Or do you always use importlib? What is the fallback scenario? I'm not familiar with importlib, if it is the only option for importing stuff, it should not be optional, but baked in. |
|
In Cpython you always use the frozen importlib |
vm/src/obj/objmodule.rs
Outdated
| let name_obj = vm.new_str(self.name.clone()); | ||
| if let Some(ref dict) = &self.into_object().dict { | ||
| let mod_dict = dict.clone(); | ||
| mod_dict.set_item("__name__", name_obj, vm)?; |
There was a problem hiding this comment.
This set_item will happen each time the dict is retrieved. Is there a way to create the dict when the module is constructed? So in init or new?
|
This actually pass the tests now 🎉 🎉 🎉 A few comments:
|
What's the alternative? We need to know what class a pyclass is associated with, and the best way we have so far is to store it in a module and import it in |
|
Thanks @coolreader18 I just had some infinite loop problems and wanted to understand more the logic. About the import being slow. When running the following script: import time
start = time.time()
import os
print(time.time() - start)When I run this without the |
Codecov Report
@@ Coverage Diff @@
## master #1018 +/- ##
==========================================
+ Coverage 64.74% 64.96% +0.22%
==========================================
Files 97 97
Lines 17093 17155 +62
Branches 3811 3832 +21
==========================================
+ Hits 11067 11145 +78
+ Misses 3443 3398 -45
- Partials 2583 2612 +29
Continue to review full report at Codecov.
|
|
After looking more into the slowness of the I know this is about 1.5-2 times slower then the current import system but I believe we can optimize this at later stage. Do you guys think this is ready to be merged? |
|
This looks good to me at this point, @windelbouwman is this good to merge? |
|
Also, I'm sure that imports will speed up even more once we have pyc serialization and use |
| self.ctx.exceptions.import_error.clone(), | ||
| "__import__ not found".to_string(), | ||
| )), | ||
| pub fn import(&self, module: &str, from_list: &PyObjectRef) -> PyResult { |
There was a problem hiding this comment.
It might be good to move this whole function to the import.rs file. But that is something we can do later on.
| } | ||
| } | ||
|
|
||
| pub fn import_builtin(vm: &VirtualMachine, module_name: &str) -> PyResult { |
There was a problem hiding this comment.
This function name is slightly confusing with the existing function builtin_import in the file builtins.rs. We should come up with some better name, preferably not having the the word builtin in it. Maybe something like internal, or baked in, or rust lib.
|
Looks good to me! |
This is a work in progress on the imprtlib module. I would love to get input as early as possible so please fill free to comment. There is still work to do to make everything works but the steps are:
importlib._installwork.bootstrap_external.__import__to use the Importers insys.meta_pathA big question I am not sure about is where to initialize the importlib? In Cpython it is done as part of the lifecycle. Where do you think we should do it?