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

`__defaults__` behavior different under cpython/cython #2650

Open
mambocab opened this Issue Oct 9, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@mambocab

mambocab commented Oct 9, 2018

We use a (admittedly hacky) behavior to change default kwargs in some of our tests:

def set_default_beta_flag_true():
    defaults = list(Cluster.__init__.__defaults__)
    defaults = (defaults[:28] + [True] + defaults[29:])
    try:
        Cluster.__init__.__defaults__ = tuple(defaults)
    except:
        Cluster.__init__.__func__.__defaults__ = tuple(defaults)

(see our tests/integration/__init__.py for context)

Subsequent calls to Cluster.__init__ respect the new defaults when running under cpython 2.7.12, but not when running after cython compilation under Cython==0.28.5.

I've written a minimal test case and a script that demonstrates the behavior in this case. I've captured my local output in that gist at well. Note the difference in the second ClassWithDefaults instance.

Happy to give any further information if requested.

@TeamSpen210

This comment has been minimized.

Show comment
Hide comment
@TeamSpen210

TeamSpen210 Oct 9, 2018

Contributor

It's not really hacky, __defaults__ is a writable attribute documented to appear on functions, it's entirely reasonable to set it like that. However it's probably a bit difficult for Cython to implement the ability to change that.

Contributor

TeamSpen210 commented Oct 9, 2018

It's not really hacky, __defaults__ is a writable attribute documented to appear on functions, it's entirely reasonable to set it like that. However it's probably a bit difficult for Cython to implement the ability to change that.

@mambocab

This comment has been minimized.

Show comment
Hide comment
@mambocab

mambocab Oct 10, 2018

it's entirely reasonable to set it like that.

Fair.

However it's probably a bit difficult for Cython to implement the ability to change that.

Why is that the case?

mambocab commented Oct 10, 2018

it's entirely reasonable to set it like that.

Fair.

However it's probably a bit difficult for Cython to implement the ability to change that.

Why is that the case?

@scoder

This comment has been minimized.

Show comment
Hide comment
@scoder

scoder Oct 14, 2018

Contributor

Did you try it with the binding=True directive?

Contributor

scoder commented Oct 14, 2018

Did you try it with the binding=True directive?

@mambocab

This comment has been minimized.

Show comment
Hide comment
@mambocab

mambocab Oct 15, 2018

You're talking about the binding directive documented here? I have not.

Should I expect it to make a difference for a method (i.e., my Cluster.__init__)? The documentation specifically says it affects the behavior of "free methods".

mambocab commented Oct 15, 2018

You're talking about the binding directive documented here? I have not.

Should I expect it to make a difference for a method (i.e., my Cluster.__init__)? The documentation specifically says it affects the behavior of "free methods".

@TeamSpen210

This comment has been minimized.

Show comment
Hide comment
@TeamSpen210

TeamSpen210 Oct 15, 2018

Contributor

Binding makes Cython functions use a custom type, instead of the normal Python type for builtin methods/functions. That makes them behave identically to normal functions for bound/unbound method behavior, and also adds a __defaults__ attribute. However checking the generated C code shows it's not actually used when actually calling the method. The reason is that Cython adds the values that are set directly into the argument unpacking code it generates. It shouldn't be difficult to make object arguments do a fast lookup on the tuple, but for C-converted arguments it'll need to store those elsewhere. Perhaps add a setter which converts the C args whenever __defaults__ is set?

Contributor

TeamSpen210 commented Oct 15, 2018

Binding makes Cython functions use a custom type, instead of the normal Python type for builtin methods/functions. That makes them behave identically to normal functions for bound/unbound method behavior, and also adds a __defaults__ attribute. However checking the generated C code shows it's not actually used when actually calling the method. The reason is that Cython adds the values that are set directly into the argument unpacking code it generates. It shouldn't be difficult to make object arguments do a fast lookup on the tuple, but for C-converted arguments it'll need to store those elsewhere. Perhaps add a setter which converts the C args whenever __defaults__ is set?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment