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

[aio types] Fix some grpc.aio python types #32475

Merged
merged 4 commits into from May 15, 2023

Conversation

Tasssadar
Copy link
Contributor

With these, it is actually possible to have typed client stubs where the return type is correctly inferred.

It's only for the non-streaming calls, because there is RequestIterableType for the streaming ones (but it's just Any with extra steps and would require much more work).

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 24, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: Tasssadar / name: Vojtěch Boček (47531bf)

@XuanWang-Amos
Copy link
Contributor

Hi, thanks for the PR, the change looks fine to me except for one concern about changing Awaitable to Generator since it's not backward compatible and might break some peoples integration. Can you share more context on the issue you're having now and how this PR can help address it?

@Tasssadar
Copy link
Contributor Author

Hello, thanks for taking a look.

The Awaitable type for __await__ is wrong. As per mypy docs and PEP492, it's supposed to return a Generator, and it simply does not work (typecheck nor infer return type) with Awaitable.

Besides, if you look at the actual implementation of __await__, that's what the method is returning, the Awaitable was never correct: https://github.com/grpc/grpc/pull/32475/files#diff-986f9aff481adca09f2b4e49c2d5daa8971a6c5e113bdd96002681e208c0aa1fR268-R293

Now, as for what I'm trying to achieve: mypy-protobuf generates type stubs like this for gRPC services:

class FooServiceStub:
    def __init__(self, channel: grpc.Channel) -> None: ...

    GetFoo: grpc.UnaryUnaryMultiCallable[FooRequest,FooResponse]

It generates non-aio stubs, true, but if you put import grpc.aio as grpc on top, plus the changes in this PR, it actually typechecks and infers argument and return value types correctly.

@XuanWang-Amos XuanWang-Amos added release notes: yes Indicates if PR needs to be in release notes kokoro:run labels Mar 2, 2023
@XuanWang-Amos
Copy link
Contributor

Hi, sorry for late reply and thanks for the additional context.

We agree with the proposed change, but we would like to verify that it does not break any mypy or pytype checks before merging it. We will update this thread once we had a chance to verify that.

In the meantime, please let us know if the change has already been tested.

@Tasssadar
Copy link
Contributor Author

Hi, thank you. I'm already using this in some projects, but they are not open-source so I can't provide any examples unfortunately.

@XuanWang-Amos
Copy link
Contributor

Sorry about the delay. I just verified with both mypy and pytype, good news is we're now sure this change is good to merge, but bad news is we're not checking aio files in our pytype workflow, we might need fix that later.

I added a small comment, can you help address that and fix merge conflict?

Thanks again for the contribution!

@Tasssadar
Copy link
Contributor Author

Hello, sorry for the long turnaround, but I added the Generic and rebased the PR.

@XuanWang-Amos
Copy link
Contributor

Looks like some bazel tests are failing, you can check them locally using:
bazel test //src/python/grpcio_tests/tests/unit:all.
For sanity (format) related fixes, you can run:
./tools/distrib/sanitize.sh (Or ./tools/distrib/yapf_code.sh and ./tools/distrib/isort_code.sh etc manually).

@XuanWang-Amos XuanWang-Amos changed the title Fix some grpc.aio python types [aio types] Fix some grpc.aio python types May 11, 2023
@XuanWang-Amos XuanWang-Amos merged commit 5bd38df into grpc:master May 15, 2023
61 of 62 checks passed
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label May 16, 2023
eugeneo pushed a commit to eugeneo/grpc that referenced this pull request May 17, 2023
With these, it is actually possible to have typed client stubs where the
return type is correctly inferred.

It's only for the non-streaming calls, because there is
`RequestIterableType` for the streaming ones (but it's just Any with
extra steps and would require much more work).

---------

Co-authored-by: Xuan Wang <xuanwn@google.com>
wanlin31 pushed a commit that referenced this pull request May 18, 2023
With these, it is actually possible to have typed client stubs where the
return type is correctly inferred.

It's only for the non-streaming calls, because there is
`RequestIterableType` for the streaming ones (but it's just Any with
extra steps and would require much more work).

---------

Co-authored-by: Xuan Wang <xuanwn@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imported Specifies if the PR has been imported to the internal repository lang/Python release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants