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

PEP 646: Add explicit section on endorsements #2055

Merged
merged 6 commits into from
Sep 17, 2021
Merged

PEP 646: Add explicit section on endorsements #2055

merged 6 commits into from
Sep 17, 2021

Conversation

mrahtz
Copy link
Contributor

@mrahtz mrahtz commented Aug 22, 2021

As suggested by @gvanrossum.

@fylux Anything else to add here - should we ask the JAX folks you talked to whether they want to contribute a statement for the PEP? And any word from Bas?

@pradeep90 too.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Looks like you need to do some reformatting to make this valid REST. (I’d suggest using some markup that makes it looks like a quote.)

@mrahtz
Copy link
Contributor Author

mrahtz commented Aug 23, 2021

Oops - fixed.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

LGTM! Let me know if you want me to merge this now or if you're waiting for feedback from others.

pep-0646.rst Outdated
Endorsements
============

This purpose of this PEP is to make life easier for the numerical computing
Copy link
Contributor

Choose a reason for hiding this comment

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

While numerical computing is a key motivation, I don't think it is the only purpose of the PEP. As the numerical computing folks have pointed out, this PEP would be worth having even for non-Tensor purposes. The PEP itself links to a python/typing issue with many use cases.

Perhaps we can clarify that a bit here. Otherwise, it reads like Tensor authors are the only, narrow set of users for this feature and this feature would live or die by adoption from the few libraries, which is not true.

For example, there are plenty of real-world callbacks that expect a variadic *args:

def call_soon(callback: Callable[[*Ts], R], *args: *Ts) -> R:
    ...
    return callback(*args)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. What do you think of the revised wording?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice wording!

pep-0646.rst Outdated
Comment on lines 1174 to 1175
community. How likely is it that numerical computing libraries will actually
make use of the features proposed in this PEP?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's worth clarifying that variadic tuples will be useful in Tensor library stubs, not their official implementations. In other words, even if PyTorch, NumPy, etc. never use variadic tuples in their source code, users would still benefit from having stubs with variadic tuples. For the purposes of typechecking, what matters is the stub. These libraries are often written in C++ anyway, meaning that they aren't really the target users. The main targets are people who use Tensors in their code and want to catch shape errors up front.

So, while it is nice to have endorsements from Tensor libraries, I don't see it as make-or-break for Tensor shape types. The community will be maintaining a set of stubs anyway, like we began in pytorch_examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true, but if the steering council are sceptical, I think this argument won't sway them: it would be strange for a language to adopt a controversial language feature only to support community stubs. I think the weight of this section depends on big library authors themselves making use of what we propose (even if it's in stubs that they provide rather than the actual implementation, considering that the implementation is mostly not Python).

@fylux
Copy link

fylux commented Aug 24, 2021

I have asked to someone in JAX if he could provide an endorsement and about Bas I am waiting for a reply.

@mrahtz
Copy link
Contributor Author

mrahtz commented Sep 7, 2021

Sorry for the slow reply - busy couple of weeks!

@mrahtz
Copy link
Contributor Author

mrahtz commented Sep 7, 2021

I have asked to someone in JAX if he could provide an endorsement and about Bas I am waiting for a reply.

@fylux Any updates?

@fylux
Copy link

fylux commented Sep 8, 2021

Bas replied that he will do it (he is catching up after holidays) but of Jax finally they feel that they do not have much to say.

@mrahtz
Copy link
Contributor Author

mrahtz commented Sep 13, 2021

Great - thanks @fylux and @ BvB93!

@gvanrossum I think this is ready to merge now.

@ambv ambv merged commit 9c79ee8 into python:master Sep 17, 2021
@mrahtz mrahtz deleted the pep646-endorsements branch January 13, 2022 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants