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

Import gRPC stubs from the grpc-stubs project #11204

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

shabbyrobe
Copy link

@shabbyrobe shabbyrobe commented Dec 27, 2023

Following on from #10020, it seemed from that discussion that there was both desire from the issue tracker on the grpc-stubs project to submit these types to typeshed, and willingness on the part of typeshed to accept them, so here's my best first effort at making that happen. Happy to sign any CLA or copyright assignment you require.

This PR attempts to massage the stubs into a hopefully acceptable shape. The original types are functional, albeit gappy. I'm not sure what typeshed's current stance is on incomplete types but I do see a fair bit of prior art in there so I presume there's at least some leeway.

Full disclosure, there's an element of handoff here in my motivation as I'm not involved in any way with any project that uses gRPC and haven't been for nearly 5 years. I'm in no position to continue as maintainer of those stubs as it is; I hoped that by submitting to typeshed that might open up the maintenance pool somewhat. If that disqualifies this from inclusion that's fair; I can't quite tell if the original submitter of typings to typeshed is expected to continue to be responsible after inclusion, but I will need to archive and deprecate grpc-stubs regardless of the outcome, as I don't have enough mental context for working with gRPC any more and no proximate opportunity to revive it.

I found while preparing this PR that what was in the grpc-sttubs project was substantially misaligned with the typeshed conventions so I've worked through all of the lints and test failures that came up while migrating. I've also ported the tests over from the approach used in grpc-stubs to the approach used in typeshed.

There were a couple of other seemingly related things I wasn't sure about. The implementation package is called grpcio; should I have bundled up the grpc* folders into a grpcio folder or are they ok where they are? Also, when using requires in my METADATA.toml, what is the correct way to refer to the base grpc package? Pretty sure I'm doing this wrong as I see I'm getting a third party subtest failure.

I am happy to spend a bit more time massaging this into a fit state for inclusion; I'm quite sure there's a fair bit that's still wrong about these (especially as they were intended to evolve alongside a project, but it just didn't pan out that way, and I ceased to be involved soon after their brisk creation, c'est la vie). They're a bit gappy in places and there were some outstanding issues on the original repo that I was not steeped enough in gRPC any more to be able to contextualise, let alone fix.

Please let me know whether it's worth proceeding and if there's anything that needs to be done, or if there's no pathway from this point and I should just archive the repo. Thank you.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@srittau
Copy link
Collaborator

srittau commented Dec 28, 2023

Thanks for the PR! The stubtest checks currently fail: https://github.com/python/typeshed/actions/runs/7343810747/job/19994850116?pr=11204. This is the relevant output, I think:

× python setup.py egg_info did not run successfully.
  │ exit code: 1
  ╰─> [6 lines of output]
      Traceback (most recent call last):
        File "<string>", line 2, in <module>
        File "<pip-setuptools-caller>", line 34, in <module>
        File "/tmp/pip-install-4nj3_q11/grpc_785350fd81[74](https://github.com/python/typeshed/actions/runs/7343810747/job/19994850116?pr=11204#step:5:75)466f9c3ae4e38035d8f1/setup.py", line 33, in <module>
          raise RuntimeError(HINT)
      RuntimeError: Please install the official package with: pip install grpcio
      [end of output]

I have no experience with grpc, so please correct me if my understanding is wrong: While these are the stubs for "gprc", what you actually install is the grpcio package? This is something that our current test setup doesn't handle, although I don't think it would be too hard to implement.

@shabbyrobe
Copy link
Author

Thanks for getting back to me so quickly. I can see the failure but not sure what to do about it. I think you're right that it boils down to the grpcio vs grpc thing. You're right that you install grpcio and get the set of packages I've added here. I wasn't sure if that meant I should put all the things I added here in a grpcio folder or if there was another mechanism, but it sounds like it might not actually be supported by the current test framework?

@AlexWaygood
Copy link
Member

I wasn't sure if that meant I should put all the things I added here in a grpcio folder or if there was another mechanism, but it sounds like it might not actually be supported by the current test framework?

I think just putting the stubs in a grpcio folder should be fine. It'll mean the stubs will be uploaded to PyPI as types-gprcio rather than as types-grpc, but I don't think there'd be any other issues (though I might be missing something)

@srittau
Copy link
Collaborator

srittau commented Dec 28, 2023

I haven't thought too deeply about it, but usually our stubtest checks install the package "x" when building the stubs in the "x" directory (the package will be called "types-x"). We could add a field like packages in the tool.stubtest table of METADATA.toml that will override this default.

On a related note: It would make sense to me to also add (dummy) stubs for grpcio* etc. that depend on the grpc* stubs so that users would install grpcio types-grpcio instead of grpcio and types-grpc. But that should probably done in a separate PR so we can work out the kinks with this one separately.

@shabbyrobe
Copy link
Author

Putting it in grpcio worked @AlexWaygood, I am seeing useful output in the result of stubtest now. Looks like there's quite a few things for me to work through there, I'll pick off that list and then see what's left that I might need a hint or two with and report back.

This comment has been minimized.

@shabbyrobe
Copy link
Author

shabbyrobe commented Dec 28, 2023

Interesting, it looks like the typings in the aio package are reasonably complete. I don't remember that always being the case, but I might be misremembering. I've been pretty hands-off with grpc for a good long while now. I'm inclined to remove the aio typings from this PR, rather than fuss over all those stubtest warnings.

Does typeshed play nice when part of a package is typed well and doesn't require stubs but another part of it isn't, and does?

This comment has been minimized.

Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

spark (https://github.com/apache/spark)
- python/pyspark/sql/connect/utils.py:72: error: Cannot find implementation or library stub for module named "grpc_status"  [import-not-found]
- python/pyspark/sql/connect/client/reattach.py:31: error: Cannot find implementation or library stub for module named "grpc_status"  [import-not-found]
- python/pyspark/sql/connect/client/core.py:54: error: Cannot find implementation or library stub for module named "grpc_status"  [import-not-found]

jax (https://github.com/google/jax)
- jax/experimental/jax2tf/examples/serving/model_server_request.py:18: error: Unused "type: ignore" comment, use narrower [import-not-found] instead of [import] code  [unused-ignore]
+ jax/experimental/jax2tf/examples/serving/model_server_request.py:18: error: Unused "type: ignore" comment  [unused-ignore]

@Avasam
Copy link
Sponsor Collaborator

Avasam commented Apr 23, 2024

Does typeshed play nice when part of a package is typed well and doesn't require stubs but another part of it isn't, and does?

Yes! (at least it should, we don't have a lot of use-cases). Check the entry partial_stub under https://github.com/python/typeshed/blob/main/CONTRIBUTING.md#the-metadatatoml-file Typeshed can generate PEP 561 Partial stub packages. It's made exactly for that purpose.

In the case the gRPC stubs are complete, but with modules purposefully left to the original package, you could set ignore_missing_stub = false, partial_stub = true and use stubtest entries to explicitly ignore said modules.

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.

4 participants