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
Simplify caching of modules with %run #3555
Conversation
Previously, we cleared and re-used a single FakeModule instance in which to run cells, and cached copies of the modules' namespaces to prevent them from being cleared. Now, we cache one FakeModule instance per script file, clearing it and re-using it if the same script is re-run. This seems more robust. Closes ipythongh-3547
Thanks, this all looks good to me, and cleaner than before. I will defer to @fperez as someone more familiar with |
Since this is a breaking API change, I've added a note to the What's New doc in case any poor soul is calling this. |
Explicitly assigning @fperez for his feedback. |
@@ -819,16 +819,9 @@ def register_post_execute(self, func): | |||
# Things related to the "main" module | |||
#------------------------------------------------------------------------- | |||
|
|||
def new_main_mod(self,ns=None): | |||
def new_main_mod(self,filename): | |||
"""Return a new 'main' module object for user code execution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should document the filename
parameter in the docstring properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
I believe there's a problem with the logic, pointed above. Let's not merge this until we can either clarify that I'm wrong, or fix the issue if I'm right. |
Simplify caching of modules with %run. Previously, we cleared and re-used a single FakeModule instance in which to run scripts, and cached copies of the modules' namespaces to prevent them from being cleared. Now, we cache one FakeModule instance per script file, clearing it and re-using it if the same script is re-run. Closes #3547, and fixes another test that was marked as a known failure.
Drop fakemodule After PR #3555, I don't think we have any need for our FakeModule class, so this removes it in favour of just using ModuleType. There shouldn't be any user-visible benefit, so we could leave this until after release if we're going into stability mode. On the other hand, it changes an (undocumented) API, so we might prefer to do this before 1.0.
Simplify caching of modules with %run. Previously, we cleared and re-used a single FakeModule instance in which to run scripts, and cached copies of the modules' namespaces to prevent them from being cleared. Now, we cache one FakeModule instance per script file, clearing it and re-using it if the same script is re-run. Closes ipython#3547, and fixes another test that was marked as a known failure.
Drop fakemodule After PR ipython#3555, I don't think we have any need for our FakeModule class, so this removes it in favour of just using ModuleType. There shouldn't be any user-visible benefit, so we could leave this until after release if we're going into stability mode. On the other hand, it changes an (undocumented) API, so we might prefer to do this before 1.0.
Previously, we cleared and re-used a single FakeModule instance in which to run scripts, and cached copies of the modules' namespaces to prevent them from being cleared. Now, we cache one FakeModule instance per script file, clearing it and re-using it if the same script is re-run.
I've assumed that
new_main_mod
andcache_main_mod
don't count as public APIs; they're certainly bizarre corners that I hope no-one else has needed to use directly. Fixing this would be much harder if we have to preserve their signatures.This closes #3547, and also fixes another test that was marked as a known failure. (Entirely clean test run on IPython.core!)
Pinging @fperez, as I think you're the most familiar with this code.