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

Async Stub Annotations #611

Merged
merged 7 commits into from
Oct 21, 2024

Conversation

artificial-aidan
Copy link
Contributor

  • Generate a set of TypeVars and a generic class
  • TypeVars have defaults to match expected behavior
  • Overload init methods to get expected types back
  • Create AsyncStub type alias

This was a fun one, I think I've maintained existing behavior, while made it better to use. Initializing with an async channel will now get you the correct type without casting.

I found an issue with the negative tests, there seems to be some ambiguity about the note that mypy is returning, so it intermittently fails

@artificial-aidan
Copy link
Contributor Author

See #536 for more discussion

@artificial-aidan
Copy link
Contributor Author

As I always say, the answer is always in github issues

python/mypy#16363

I added --no-incremental to the mypy flags when testing for now.

@artificial-aidan
Copy link
Contributor Author

@nipunn1313 bumping this back to the top.


def __init__(self, channel: typing.Union[grpc.Channel, grpc.aio.Channel]) -> None: ...
UnaryUnary: grpc.UnaryUnaryMultiCallable[
_MTVDummyService0 = typing_extensions.TypeVar(
Copy link
Owner

Choose a reason for hiding this comment

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

don't love the name here. Is there a way to make the naming based on the service name / method name - rather than these iterating numbers?

Perhaps instead of {i} use the {method.name}

MTV also feels rather opaque. "MethodTypeVar" isn't particularly useful to the enduser (who will probably see these types when hovering around in vscode).

Perhaps we don't need the "_MTV" prefix at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's been a while so I'm going to have to remember why I did the naming like that. I know I wasn't a huge fan of it, but wanted to get something down on paper. Let me see what it looks like to not have the prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok here we go. I had to keep the leading underscore because of stubtest (left a comment with the SO explaining it, couldn't actually find it in stubtest docs, didn't look too long). So now it is just _ServiceNameMethodNameType which shouldn't conflict. Also cleaned it up so the typevar name building so it is more maintainable.

@@ -53,7 +53,7 @@ async def test_grpc() -> None:
server = make_server()
await server.start()
async with grpc.aio.insecure_channel(ADDRESS) as channel:
client: dummy_pb2_grpc.DummyServiceAsyncStub = dummy_pb2_grpc.DummyServiceStub(channel) # type: ignore
client = dummy_pb2_grpc.DummyServiceStub(channel)
Copy link
Owner

Choose a reason for hiding this comment

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

nice!

@nipunn1313
Copy link
Owner

sorry for the delay - I hadn't been focusing on this for a while (I'm unlikely to be highly responsive, but this very-long-wait isn't expected in general - aplogies).

The PR now requires a rebase.

On the whole this looks - great. Some requests.
Could you update CHANGELOG.md in the upcoming section? Would also love to have you add yourself to the contributors section of README.md in the PR as well if you are ok with it.

TypeVars have defaults to match expected behavior

Overload init methods to get expected types back

Create AsyncStub type alias

Signed-off-by: Aidan Jensen <[email protected]>
Signed-off-by: Aidan Jensen <[email protected]>
@artificial-aidan artificial-aidan force-pushed the aidan/async-stub-overload branch from 501d032 to 883de94 Compare October 21, 2024 02:38
Signed-off-by: Aidan Jensen <[email protected]>
Signed-off-by: Aidan Jensen <[email protected]>
Signed-off-by: Aidan Jensen <[email protected]>
@artificial-aidan
Copy link
Contributor Author

sorry for the delay - I hadn't been focusing on this for a while (I'm unlikely to be highly responsive, but this very-long-wait isn't expected in general - aplogies).

The PR now requires a rebase.

On the whole this looks - great. Some requests. Could you update CHANGELOG.md in the upcoming section? Would also love to have you add yourself to the contributors section of README.md in the PR as well if you are ok with it.

Rebased, let me know if you want me to squash or clean up any before you merge.

@nipunn1313
Copy link
Owner

Ok let's do it!
I am probably going to wait a little bit before releasing again (try to get one more fix in)

@nipunn1313 nipunn1313 merged commit 54d184c into nipunn1313:main Oct 21, 2024
6 checks passed
@tmickel
Copy link

tmickel commented Oct 21, 2024

Appreciate this one @nipunn1313 and @artificial-aidan! Looks promising for good grpc Python types

@artificial-aidan
Copy link
Contributor Author

@nipunn1313 bump for a release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants