Skip to content

Revert __signature__ to being a cache, remove string and callable support #116110

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

Closed
Gouvernathor opened this issue Feb 29, 2024 · 10 comments
Closed
Labels
type-feature A feature request or enhancement

Comments

@Gouvernathor
Copy link
Contributor

Gouvernathor commented Feb 29, 2024

Feature or enhancement

Proposal:

#100039 modified the behavior of __signature__ as used by the inspect.signature function :

  • if the fields contains a Signature object, it is returned by the function (that doesn't change)
  • if it contains None, it is ignored (that doesn't change)
  • if it contains a string, it is converted to a signature (new)
  • if it contains a callable, it is called with no parameters and expected to return either a signature object or a string which will be treated as the above (new)
  • if it contains any other value, a TypeError is raised (this used to include strings and callables, it doesn't anymore)

As for strings :
The __text_signature__ attribute's purpose is to pass a string to inform the signature. It's also only used for C-written functions, and iirc there's no way to use it in pure python : I understand that, but if the ability to specify a text override of inspect.signature must happen, then it should go through __text_signature__ and not __signature__.

As for callables :
The Signature object qualifies, and the signature function inspects, callables. So, if when asked for the signature of a given callable you return another callable, it would make more sense to consider the second callable's signature as the signature of the first callable, than considering the second callable to return the signature of the first. And why no parameters ? Why not pass the first callable as parameter ? Or the actual signature of the first callable ? That implementation raises a lot of questions of arbitrariness, which makes it a bad design in my opinion.

As for the purpose :
The use case, which was adding signature overrides to Enums, does not require any change to the inspect module, in fact a perfectly good alternative implementation can be found in #115937. It should also make for a considerably faster execution.

I suggest reverting these changes.

Links to previous discussion of this feature:

This was discussed in #115937 and #116086 in which it was advised to separate the question of the implementation from that of the documentation.

Linked PRs

@Gouvernathor Gouvernathor added the type-feature A feature request or enhancement label Feb 29, 2024
@skirpichev
Copy link
Member

Perhaps, #115984 may be an alternative implementation. But this pr was rejected.

@Gouvernathor
Copy link
Contributor Author

Gouvernathor commented Feb 29, 2024

My sentiment reading #116086 is that it was only closed in favor of the pure-doc PR. But there was no actual argument formulated against the merits of your PR, so I think it should be merged as fixing this issue.

@merwok
Copy link
Member

merwok commented Feb 29, 2024

I think the discussion taking place should reach a conclusion before more changes are done
https://discuss.python.org/t/signatures-for-extension-modules-pep-draft/43914

@skirpichev
Copy link
Member

there was no actual argument formulated against the merits of your PR

Yes, I would appreciate, if @erlend-aasland could do this. I don't think we should reopen this pr without permission from some core developer.

My guess is, that this will introduce some incompatibility. But on another hand, the documentation now says explicitly, that we can break things here. So, I would prefer #115984 for a simpler logic.

the discussion taking place should reach a conclusion before more changes are done
https://discuss.python.org/t/signatures-for-extension-modules-pep-draft/43914

I feel that the discussed proposal has a very little support, so far. But more complex logic for handling the __signature__ attribute is not too important for one.

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Mar 1, 2024

So, I would prefer #115984 for a simpler logic.

I don't have an opinion about that particular change, and I don't have the bandwidth nor desire to involve myself in such a discussion. But such a change would need to be discussed with a broader audience than on a GitHub issue. Please open a discussion on Discourse.

@skirpichev
Copy link
Member

I don't have an opinion about that particular change

So, in principle, you will not object if I reopen mentioned PR as a solution for this issue?

skirpichev added a commit to skirpichev/cpython that referenced this issue Mar 1, 2024
@erlend-aasland
Copy link
Contributor

[...] you will not object if I reopen mentioned PR as a solution for this issue?

I will not object. Anyone is free to open and re-open PRs. Of course, it is good practice not to repeatedly try to push a change that has been rejected1, as that is seen as pushy or demanding behaviour. I still think your change needs a wider discussion (for example on Discourse).

Footnotes

  1. generally speaking, not specifically regarding your PR

@skirpichev
Copy link
Member

I will not object.

Thanks.

I still think your change needs a wider discussion (for example on Discourse).

@Gouvernathor opened d.p.o discussion about docs, I just left comment on possible changes in logic. I hope, it's better than starting a new thread.

@Gouvernathor
Copy link
Contributor Author

Gouvernathor commented Mar 1, 2024

I think it's better to a new discourse thread, separate both from this issue and the pure-doc one I opened (since it's located in the documentation part of the forum, it's not really the place to discuss implementation changes).
I also think it would be better not to have two discussions in parallel, so it should probably wait for the other discussion (with the PEP draft) to settle. So, I can either open it now and present it as "on hold", or open it after the other discussion is done. Probably the former, and I'll @ you in it anyway.

I suggest you reopen your PR as a draft (so it doesn't grab attention as something to be reviewed), and link it to this issue if possible.

skirpichev added a commit to skirpichev/cpython that referenced this issue Mar 2, 2024
skirpichev added a commit to skirpichev/cpython that referenced this issue Jul 19, 2024
@skirpichev
Copy link
Member

FYI: https://discuss.python.org/t/66906

efimov-mikhail pushed a commit to efimov-mikhail/cpython that referenced this issue Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

5 participants