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

allow type of already existing class instances to be updated #11644

Merged
merged 5 commits into from Jun 18, 2019
Merged

allow type of already existing class instances to be updated #11644

merged 5 commits into from Jun 18, 2019

Conversation

@daharn
Copy link
Contributor

@daharn daharn commented Mar 11, 2019

This is my first attempt in solving the problematic discussed in #11588. Although %autoreload updates class attributes for reloaded modules "on the fly", thus providing these updated attributes also to previously instantiated objects of that class, these old objects remain of type "old class definition", meaning that they can't be pickled.

(This is also my first open source contribution in general, so be gentle with me :) )

As you can see, this relies on identifying the execution frame from the stack that holds all user defined variables. My current search conditions were inspired by the definition of %who_ls, but even though it worked for manual tests (creating a module with a class, creating an instance of that class, change the definition inside the module, compare type of already existing instance with new ones), I couldn't get it to work in the FakeShell environment of the tests in test_autoreload. So maybe there is a more general/fail proof possibility to find the user namespace?

EDIT: The tests fail mainly because FakeShell doesn't have a user_ns. Adding an empty namespace lets the tests succeed, but the functionality is still lost.

@Carreau
Copy link
Member

@Carreau Carreau commented Mar 11, 2019

Thanks for your contribution, and congrats, I know it is a sometime scary step !
You did (almost) everything right; you'll just see people ask you to not send the PR from master of your fork, but use a feature-branch. Though that's a detail and it does not matter for now.

I'll try to come back to review this as soon as possible, though I'm quite busy this week, so I can't promise.

From a quick read that looks great, I just need to understand it, and find out how to fix the tests.

Thanks !

Loading

@Carreau
Copy link
Member

@Carreau Carreau commented Mar 19, 2019

Starting to look into this.

Loading

IPython/extensions/autoreload.py Outdated Show resolved Hide resolved
Loading
There seem to have been some infinite recursion in the previous version
of the code, so implement a more classical graph finding algorithm.

This should still be properly tested
@Carreau
Copy link
Member

@Carreau Carreau commented Mar 19, 2019

Would you mind having a look at this new version ? I proposed an alternative with less recursion, which appear to work on my machine.

Test and docs are still needed though, and code need to be cleaned up.

Loading

@Carreau
Copy link
Member

@Carreau Carreau commented Mar 19, 2019

Hey ! Basic test seem to pass. Sounds like a good start.

@daharn how familiar are you with git/github ? Will you manage to get my changes on your machine, and can take care of (some-of) the cleaning documenting ? Or do you need some extra guidance ?

Loading

@daharn
Copy link
Contributor Author

@daharn daharn commented Mar 21, 2019

Thanks for looking into this!
I am fairly familiar with git although I haven't used github much. Getting your changes shouldn't be a problem. I will let you know once I got around to look into them.

Loading

@Carreau
Copy link
Member

@Carreau Carreau commented Mar 21, 2019

Thanks ! Much appreciated.

Loading

IPython/extensions/autoreload.py Outdated Show resolved Hide resolved
Loading
… avoid infinite recursion.

This should pass the already existing autoreload tests, however, new tests for this feature still need to be implemented.
@daharn
Copy link
Contributor Author

@daharn daharn commented Mar 31, 2019

Next attempt. I kind of stuck to my original recursion but tried to include your suggested checks to avoid revisiting of already browsed objects.

This passes the tests and does a good job on updating all instance references in the user namespace (a notable exception are instances yielded from a generator. I don't know whether it would even be possible to update them).

Let me know what you think and if you have an idea on how to add some tests for this feature in the autoreload tests.

Loading

@Carreau
Copy link
Member

@Carreau Carreau commented Apr 1, 2019

Thanks for this, I'll try to look at it soon.

Loading

@Carreau
Copy link
Member

@Carreau Carreau commented Apr 19, 2019

I'm still planning to have a look. Apologies for the delay

Loading

@daharn
Copy link
Contributor Author

@daharn daharn commented Apr 20, 2019

No worries, we are all doing this in our spare time I guess.

Loading

@Carreau
Copy link
Member

@Carreau Carreau commented Jun 18, 2019

I wrote and push test for this functionality.

You can run cd ; iptest IPython.extensions.tests.test_autoreload:TestAutoreload.test_reload_class_type; cd - to test just the behavior that the __class__ has been updated.

Loading

@Carreau
Copy link
Member

@Carreau Carreau commented Jun 18, 2019

Well, took little over 3 month; so thanks for your patience; but that looks god to me; I removed one more debug statement and will wait for tests to pass then merge. I need to remember to update the what's new to give an example of the new possibility.

Loading

@Carreau Carreau merged commit bd5753b into ipython:master Jun 18, 2019
4 checks passed
Loading
@Carreau Carreau added this to the 7.6 milestone Jun 18, 2019
@Carreau Carreau mentioned this pull request Jun 18, 2019
@daharn
Copy link
Contributor Author

@daharn daharn commented Jun 24, 2019

Thanks for including this. Keep up the great work!

Loading

@Carreau
Copy link
Member

@Carreau Carreau commented Aug 12, 2019

Can you check #11849 for your use case ? I'm hoping this will still work properly and potentially be faster.

Loading

@daharn
Copy link
Contributor Author

@daharn daharn commented Aug 17, 2019

I will as soon as I am able to.

If this iterating through workspace turns out to be unsuited in general, a new (and arguably more elegant approach) would be to maintain a list of all objects for any custom module and add each object to it, when it is created.

Maybe this could be done by messing with the definition of __new()__...?

Loading

@Carreau
Copy link
Member

@Carreau Carreau commented Aug 17, 2019

I think there might be a better way.

Usually obj.__class__ point to the class of the object, to technically all instances of Foo are in gc.get_referers(Foo), so we should be able to just check in gc.get_referers(OldClass). I"d like to avoid messing with __new__ as much as I can.

Loading

@wmmxk
Copy link

@wmmxk wmmxk commented Aug 22, 2019

The %load_ext autoreload magic is very handy when I work in a dual development mode: Jupyter+IDE.
I need to add "%load_ext autoreload" at the beginning of each jupyter notebook. Is there a way to set this configuration permanently? I know there is "~/.ipython/profile_default" folder, so I think it is possible but I am not sure.

Loading

@Carreau
Copy link
Member

@Carreau Carreau commented Aug 23, 2019

Loading

@daharn
Copy link
Contributor Author

@daharn daharn commented Aug 23, 2019

I think there might be a better way.

Usually obj.__class__ point to the class of the object, to technically all instances of Foo are in gc.get_referers(Foo), so we should be able to just check in gc.get_referers(OldClass). I"d like to avoid messing with __new__ as much as I can.

I have tinkered with gc.get_referers() a little and this seems to be by far the cleanest solution. Any reason against implementing it this way? Apart from routinely evoking a function of which the documentation says "Avoid using get_referrers() for any purpose other than debugging."?

Loading

@Carreau
Copy link
Member

@Carreau Carreau commented Aug 23, 2019

I have tinkered with gc.get_referers() a little and this seems to be by far the cleanest solution. Any reason against implementing it this way?

Not particularly; I was just conservative in case get_referrers did not get all objects; but if it works I don't see any reason not to use this one. Mostly I don't want to break your use case; but if that works for you I'll likely update this to use gc.get_referers(...)

Loading

@daharn
Copy link
Contributor Author

@daharn daharn commented Sep 5, 2019

I gave it a try and it works quite well. Don't know if gc.get_referrers() has a performance advantage compared to gc.get_objects(), although the amount of objects to actively crawl is far less. Maybe you can test this in one of the more workspace demanding environments: #11876

Loading

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

Successfully merging this pull request may close these issues.

None yet

3 participants