Check to see if correct source for decorated functions can be displayed #578

Closed
wants to merge 4 commits into
from

Projects

None yet

3 participants

@bjedwards
Contributor

If a function is decorated with a decorator which itself has been
decorated using @decorator, currently the no source code is
displayed when '??' is used. This checks to see if the function
has a wrapped attribate (added by @decorator) and appropriately
displays the source for that object.

Ben Edwards Display source code correctly for decorated functions.
If a function is decorated with a decorator which itself has been
decorated using @decorator, currently the no source code is
displayed when '??' is used. This checks to see if the function
has a __wrapped__ attribate (added by @decorator) and appropriately
displays the source for that object.
1b4b159
@minrk
Member
minrk commented Jul 14, 2011

Nice!

This will actually make Pyro objects, and others that lie to hasattr, worse, but I'm not sure there's a good answer for that.

@takluyver
Member

I see functools.update_wrapper also adds a __wrapped__ attribute, so this looks sensible.

One consideration - how does this work if a wraps b and b wraps c. Is a.__wrapped__ a reference to c, or to b? If it's a reference to b, should we make that if into a while, so that we unwrap as many times as needed?

@bjedwards
Contributor

We could drop it into a loop to try to dig down, that's probably a good
idea...

On Thu, Jul 14, 2011 at 12:49 PM, takluyver <
reply@reply.github.com>wrote:

I see functools.update_wrapper also adds a __wrapped__ attribute, so
this looks sensible.

One consideration - how does this work if a wraps b and b wraps c.
Is a.__wrapped__ a reference to c, or to b? If it's a reference to b,
should we make that if into a while, so that we unwrap as many times as
needed?

Reply to this email directly or view it on GitHub:
#578 (comment)

Ben Edwards Address multiple decorators for source viewing.
Drops through multiple decorators as long as an object has a
__wrapped__ attribute
981d252
@bjedwards
Contributor

Will it make Pyro objects worse because hassattr will return True when the attribute doesn't exist, otherwise it should just fall through right? I didn't know there were objects that could lie to hasattr...

@takluyver
Member

Actually, checking with Python 3, the link is made directly, not over several steps. So:

def a():
    print 12

@wraps(a)
def b():
     a()

@wraps(b)
def c():
     b()

id(c.__wrapped__) == id(a)    # True

Python 2.7 functools doesn't add a __wrapped__ attribute. Haven't checked this with the decorator library yet.

@minrk
Member
minrk commented Jul 14, 2011

Remote objects that expose methods via attributes lie to hasattr often, because attribute resolution does not occur until you actually need the object. It makes programming certain things against them really annoying. It actually used to crash IPython entirely to inspect a Pyro object.

So in pyro:

hasattr(obj, '__wrapped__') is True
requesting obj.__wrapped__ will return a remoteMethod
calling obj.__wrapped__() will raise an AttributeError.

So obj?? will always get the source for remoteMethod, rather than DynamicProxy, or whatever subclass it might be. I'm not sure this is actually a big deal, but I just thought I would point it out.

@bjedwards
Contributor

Even if the link is made directly in Python 3, looping through will only take a moment (as long as there are not an obscene number of decorators), or is there another degeneracy I am missing?

@bjedwards bjedwards closed this Jul 14, 2011
@bjedwards bjedwards reopened this Jul 14, 2011
@takluyver
Member

No, that's fine. Although, thinking about it, could there be any cases where obj.__wrapped__ returns a reference to obj, or to an equivalent object, so that it gets stuck in an infinite loop? I can't think of any off the top of my head, just wondering.

@bjedwards
Contributor

We could just check for if they ref each other

while hasattr(obj,"__wrapped__"):
    if obj.__wrapped__ is obj:
        break
    obj = obj.__wrapped__
@bjedwards
Contributor

I guess this could still break if a->b->c->a

@takluyver
Member

That is a point, although the nature of wrapping makes it unlikely. Also, would any __getattr__ manufacture a new object of the same type, so is is False? I appreciate this is probably getting ridiculous, I'm just trying to think what's the best way to do this code. Maybe your first version is the most sensible - there's no way it can get stuck in a loop, and cases where there's several layers of wrapping are probably rare enough that we can ignore them.

Sorry if this feels like a wild goose chase - I'm just thinking aloud, trying to work out the implications of this.

@bjedwards
Contributor

It doesn't feel like a wild goose chase, this is code that is called a lot, and if there is a problem it's going to be felt. It should be right.

I don't know how often functions use more than one decorator or wrap multiple times. I would think this would start to get a bit ugly, but would be possible.

I'm new to the project, so am inclined to go with what you guys think experience wise.

@takluyver
Member

OK, I've just installed decorator 3.3.1 to check what's going on. Unlike Python 3 functools, it seems decorator only references down one layer of the wrapping.

I think we should play it safe for now, and only unwrap once - this probably covers >80% of the decorated functions people want to inspect. Let's drop Michele (the author of decorator) an email, and see if it makes sense to have a direct reference from each wrapper to the original function in the case of multiple wrappers.

Ben Edwards Revert "Display source code correctly for decorated functions."
This reverts commit 1b4b159.

Conflicts:

	IPython/core/oinspect.py
546c8fb
@minrk
Member
minrk commented Jul 18, 2011

Is this ready to go in? I'm cutting the last rc today, and we aren't going to let any more non-doc changes into 0.11 other than critical bugfixes.

I imagine we will have a relatively quick succession of 0.11.x releases, and 0.12 should be fairly soon as well.

Currently the diff actually has a conflict block from a failed merge, and a spurious test_hist.sqlite file.

Is the current decision to just revert to the first commit here?

If so, you can just do:

git reset 1b4b159ff1425bed893b3a0e1bf7a874094078b2 --hard
git push -f

to rollback subsequent commits.

@takluyver
Member

I suggest for now we just revert to the first commit, so we only unwrap once. That probably covers most cases. If there's demand for it, we can revisit this post-0.11 and work out unwrapping multiple levels.

@minrk minrk added a commit that closed this pull request Jul 18, 2011
@minrk minrk Merge branch '__wrapped__'
closes gh-578
fc0a73f
@minrk minrk closed this in fc0a73f Jul 18, 2011
@minrk
Member
minrk commented Jul 18, 2011

we can revisit this with a more clever approach in the future, but for now the initial commit is an improvement for ~all real cases. Merged as such.

@bjedwards
Contributor

Thanks for taking of the commit. I am out of the country and son'have
reliable internet.

Ben

On 7/19/11, minrk
reply@reply.github.com
wrote:

we can revisit this with a more clever approach in the future, but for now
the initial commit is an improvement for ~all real cases. Merged as such.

Reply to this email directly or view it on GitHub:
#578 (comment)

Ben

Think Easy, Light, Smooth, and Fast...You won't have to worry about the last
one--you get the first three and you'll be fast.

@mattvonrocketstein mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
@minrk minrk Merge branch '__wrapped__'
closes gh-578
16bd9cb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment