Skip to content
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

fix globals #308

Closed
wants to merge 1 commit into from
Closed

fix globals #308

wants to merge 1 commit into from

Conversation

impact27
Copy link
Contributor

@impact27 impact27 changed the base branch from master to 2.x August 17, 2021 15:30
@ccordoba12
Copy link
Member

@impact27, I think the failures in our tests are related to this: ipython/ipykernel#736

main_mod = ipython_shell.new_main_mod(
self.filename, '__main__')
self.ns_globals = main_mod.__dict__
if shell_namespace is global_namespace:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How efficient is this comparison when there are thousands of variables in the namespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the number of variables doesn’t impact the comparison speed, my understanding is that you are just checking if the pointers are the same.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, good to know.

self._saved_globals = shell_namespace.copy()
# Here we clear all variables, let's hope ipykernel
# is not too mad
shell_namespace.clear()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IPython saves several hidden variables that don't come from running code. This could break that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but there shouldn’t be any ipython code running between enter and exit. All the code runs in the same thread does it not? I agree that this is a risk

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but there shouldn’t be any ipython code running between enter and exit. All the code runs in the same thread does it not?

Yeah, I think so.

I agree that this is a risk

Have you tested this for a while to see if it introduces problems?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to call get_ipython().init_user_ns after clearing the namespace to add the hidden variables back.

@impact27 impact27 closed this Oct 2, 2021
@impact27
Copy link
Contributor Author

impact27 commented Oct 2, 2021

I am not confident enough about not introducing a bug with that one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

strange behavior with cell
2 participants