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

Bump protobuf to 4.21.*; delete google/__init__.pyi #8360

Merged
merged 6 commits into from Nov 25, 2022

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Jul 22, 2022

Release: https://pypi.org/pypi/protobuf/4.21.9
Homepage: https://developers.google.com/protocol-buffers/

If stubtest fails for this PR:

  • Leave this PR open (as a reminder, and to prevent stubsabot from opening another PR)
  • Fix stubtest failures in another PR, then close this PR

Note that you will need to close and re-open the PR in order to trigger CI

@hauntsaninja
Copy link
Collaborator

Hmm, it appears actions do not trigger unless you close and open the PR manually :-/

@github-actions

This comment has been minimized.

@hauntsaninja
Copy link
Collaborator

As mentioned in #8403 (comment) , a major version change is a good opportunity to drop the __init__.py , see #7519

@AlexWaygood
Copy link
Member

As mentioned in #8403 (comment) , a major version change is a good opportunity to drop the __init__.py , see #7519

It also sounds like pyright might be considering patching the protobuf stubs they bundle: microsoft/pyright#3751

@hmc-cs-mdrissi
Copy link
Contributor

Pyright patching it is good for users that don't install types-protobuf (maybe is majority).

If someone has types-protobuf in there dev-requirements they'll override bundled stubs and be confused. My understanding of non stdlib stubs is users are recommended to specify them as a dependency? You could treat protobuf as special and recommend rely on bundled ones, but I'd guess it'd confuse most users given it's a fairly niche bug.

@AlexWaygood
Copy link
Member

Pyright patching it is good for users that don't install types-protobuf (maybe is majority).

If someone has types-protobuf in there dev-requirements they'll override bundled stubs and be confused. My understanding of non stdlib stubs is users are recommended to specify them as a dependency? You could treat protobuf as special and recommend rely on bundled ones, but I'd guess it'd confuse most users given it's a fairly niche bug.

Ah that's a v good point. We should probably make the change here regardless of what pyright does with respect to patching the bundled version, in that case, then.

@AlexWaygood AlexWaygood added the help wanted An actionable problem of low to medium complexity where a PR would be very welcome label Aug 20, 2022
@github-actions

This comment has been minimized.

@JelleZijlstra
Copy link
Member

For the record I didn't force-push anything. I think it's because I created the SSH key that stubsabot now uses.
Screen Shot 2022-10-05 at 11 54 54 AM

@github-actions

This comment has been minimized.

@AlexWaygood AlexWaygood removed the help wanted An actionable problem of low to medium complexity where a PR would be very welcome label Oct 9, 2022
@github-actions

This comment has been minimized.

@JelleZijlstra
Copy link
Member

CI is green, remind me why we aren't merging this PR?

@hauntsaninja hauntsaninja added the status: deferred Issue or PR deferred until some precondition is fixed label Oct 19, 2022
@hauntsaninja
Copy link
Collaborator

I think the plan is to make use the opportunity of the major version bump of protobuf removes the __init__.pyi. This caused a lot of problems when we tried it earlier / users mentioned it would have been a little less surprising with a major version bump. And then removing the __init__.pyi is least disruptively done once mypy 0.990 is out (because it changes the default of --namespace-packages).

@github-actions

This comment has been minimized.

Release: https://pypi.org/pypi/protobuf/4.21.9
Homepage: https://developers.google.com/protocol-buffers/

If stubtest fails for this PR:
- Leave this PR open (as a reminder, and to prevent stubsabot from opening another PR)
- Fix stubtest failures in another PR, then close this PR

Note that you will need to close and re-open the PR in order to trigger CI
@github-actions

This comment has been minimized.

@AlexWaygood AlexWaygood removed the status: deferred Issue or PR deferred until some precondition is fixed label Nov 10, 2022
@AlexWaygood
Copy link
Member

I tried deleting google/__init__.py, but mypy doesn't seem to like it (at least not in our CI), despite --namespace-packages now being the default in mypy 0.990. Anybody have any idea what's going on there?

@github-actions

This comment has been minimized.

@nipunn1313
Copy link
Contributor

I reproed. This appears to have to do with --explicit-package-bases
https://mypy.readthedocs.io/en/stable/running_mypy.html#mapping-file-paths-to-modules

I was able to get things to pass by adding --explicit-package-bases to the tests, and adding MYPYPATH=stubs/protobuf. This is probably not an ideal solution.

It is probably a bug inside mypy regarding how --custom-typeshed-dir works with relation to namespace packages.

@hauntsaninja
Copy link
Collaborator

Nipunn is correct that we need to use --explicit-package-bases and add all stubs to MYPYPATH (or copy files to a different kind of directory structure and check them). I don't think there's an interaction with --custom-typeshed-dir, that should just be used for stdlib stubs these days (and maybe mypy_extensions).

@AlexWaygood
Copy link
Member

Nipunn is correct that we need to use --explicit-package-bases and add all stubs to MYPYPATH (or copy files to a different kind of directory structure and check them).

Will that also mean that the stubs package will be of no use to users who don't use the --explicit-package-bases option, though? That seems like it might be quite disruptive (but maybe that's okay, if we're doing a major version bump here?). Is there any possibility of --explicit-package-bases becoming the default in the future? Does it have any downsides?

[...] --custom-typeshed-dir [...] should just be used for stdlib stubs these days (and maybe mypy_extensions).

Hmm, I disagree somewhat. From time to time we have PRs to our stdlib stubs that cause mypy to fail on our third-party stubs (e.g. in #8908, the change to stdlib/datetime.pyi caused mypy to start failing on our third-party stubs for python-dateutil, meaning that those stubs had to be altered in the same PR). I'd much rather we got notified of those failures in the same PR. If we didn't use --custom-typeshed-dir for our third-party stubs, mypy would have suddenly started to fail on our stubs for python-dateutil when we next bumped mypy, due to a typeshed PR relating to the stdlib datetime stubs two months prior that nobody remembered anymore.

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Nov 23, 2022

This shouldn't have any impact on users who install stubs. The issue here is that in our CI we pass some filenames to mypy and mypy needs to convert filenames to module names somehow.

Yup, I agree with that. I was trying to respond to Nipunn's suggestion that there was a bug in --custom-typeshed-dir. mypy doesn't use --custom-typeshed-dir to find third party stubs (other than mypy_extensions maybe).

@github-actions

This comment has been minimized.

@AlexWaygood AlexWaygood changed the title [stubsabot] Bump protobuf to 4.21.* Bump protobuf to 4.21.*; delete google/__init__.pyi Nov 23, 2022
@AlexWaygood AlexWaygood marked this pull request as ready for review November 23, 2022 20:21
@github-actions
Copy link
Contributor Author

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@AlexWaygood
Copy link
Member

(I think this is mergeable now, if someone wants to take a look!)

@AlexWaygood
Copy link
Member

Let's see what happens

@AlexWaygood AlexWaygood merged commit 5453c97 into main Nov 25, 2022
@AlexWaygood AlexWaygood deleted the stubsabot/protobuf branch November 25, 2022 11:28
@@ -1,2 +1,2 @@
version = "3.20.*"
version = "4.21.*"
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to have created a 4.21.0.0 for whatever reason. It's not strictly problematic, but it's a bit weird to have

https://pypi.org/project/types-protobuf/4.21.0.0/
and
https://pypi.org/project/protobuf/4.21.9/

Copy link
Contributor

Choose a reason for hiding this comment

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

just a bit awkward to have a 4 section version for the types and a 3 section version for the regular one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I made this change to stub_uploader a month or two ago. The reason is to more clearly distinguish type versions from upstream versions and to demonstrate the intended version compatibility range of 4.21.*.
E.g., if we bumped the third number you may naively think that types-protobuf 4.21.1 or 4.21.2 corresponded to those versions of protobuf.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

ah intentional. Cool. Didn't realize. Thanks for the explanation

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.

None yet

6 participants