[For 1.2] Allow both protobuf 3 and 4 in dagster #12466

gibsondan · 2023-02-22T13:14:27Z

Summary:
This removes the protobuf3 pin in dagster and re-generates our protobuf api with protobuf 4 (which is mostly backwards compatible with protobuf3, see below)

Two downsides of this:

we need to bump our min grpcio version to 1.44 since that's the first version that is protobuf3 and protobuf4 compatible, and our min version of protobuf a bit higher. This leaves our grpcio pin fairly narrow until we can get the upper bound removed once they fix the upstream issues.
it nukes our pyright typehints for the generated modules

But users have been clamoring for us to remove this protobuf pin so I think it's worth it

vercel · 2023-02-22T13:14:31Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments

Name	Status	Preview	Comments	Updated
dagit-storybook	⬜️ Ignored (Inspect)			Mar 6, 2023 at 0:16AM (UTC)
dagster	⬜️ Ignored (Inspect)			Mar 6, 2023 at 0:16AM (UTC)

alangenfeld

hard for me to forecast what impact narrowing grpcio from 1.32.0 (Sep 9, 2020) to 1.44.0 (Feb 17, 2022) would have. Min version over a year old seems reasonable I think.

alangenfeld · 2023-02-24T15:22:13Z

python_modules/dagster/dagster/_grpc/client.py

@@ -189,12 +189,12 @@ def _streaming_query(

    def ping(self, echo: str):
        check.str_param(echo, "echo")
-        res = self._query("Ping", api_pb2.PingRequest, echo=echo)
+        res = self._query("Ping", api_pb2.PingRequest, echo=echo)  # type: ignore


what exactly are the type errors being suppressed here?

brutal - wonder what else we can do to manage this better - maybe our own layer of indirection if the mentioned external libraries are not usable.

gibsondan · 2023-02-24T16:57:15Z

I'll definitely feel better about bumping the min grpcio pin if we can get the upper bound removed.. 1.44-1.47 is a bit narrow

gibsondan · 2023-02-24T15:43:32Z

python_modules/dagster/dagster/_grpc/__generated__/api_pb2.py

-PingRequest = _reflection.GeneratedProtocolMessageType(
-    "PingRequest",
-    (_message.Message,),
-    {
-        "DESCRIPTOR": _PINGREQUEST,
-        "__module__": "api_pb2"
-        # @@protoc_insertion_point(class_scope:api.PingRequest)
-    },
-)
-_sym_db.RegisterMessage(PingRequest)


@alangenfeld i think these types were previously explicitly defined and now are generated programatically in some way that pyright can't decipher

We could separately investigate incorporating https://github.com/nipunn1313/mypy-protobuf which apparently works for pyright too, but i had some trouble getting it to work

@alangenfeld ^^ this was the response to your bump

i think these types were previously explicitly defined and now are generated programatically in some way that pyright can't decipher

Yeah you can see all those definitions removed i n the large diff of dagster._grpc.__generated__.api_pb2.

I'm kind of curious about mypy-protobuf, I can investigate it later but these ignores look fine for now.

Summary: Making dagster actually install with protobuf4 also requires removing the grpcio pin since that version is pinned to protobuf3 - so we'll need to resolve the upstream issues with hangs/segfaults before we can do this. This PR tests tox suites with both protobuf4 and protobuf3.

gibsondan · 2023-03-06T12:42:27Z

python_modules/dagster/setup.py

+        "grpcio>=1.44.0,<1.48.1",
+        "grpcio-health-checking>=1.44.0",


re - the pins here, one thing i'm not sure about: this new min version is what's needed assuming you want things to keep working if you upgrade to protobuf4, but if you leave the installed protobuf version at 3, you can keep it back at grpcio 1.32.0

i.e. these combos are valid after this PR:
grpcio 1.32.0 / protobuf 3.20.0
grpcio 1.44.0 / protobuf 3.20.0
grpcio 1.44.0 / protobuf 4.22.0

So we could technically leave the >=1.32.0 but that could still result in a combo of libraries that satisfies our constraints but doesn't load correctly

alangenfeld · 2023-03-06T17:16:31Z

python_modules/dagster/dagster/_grpc/client.py

@@ -189,12 +189,12 @@ def _streaming_query(

    def ping(self, echo: str):
        check.str_param(echo, "echo")
-        res = self._query("Ping", api_pb2.PingRequest, echo=echo)
+        res = self._query("Ping", api_pb2.PingRequest, echo=echo)  # type: ignore


brutal - wonder what else we can do to manage this better - maybe our own layer of indirection if the mentioned external libraries are not usable.

Summary: This removes the protobuf3 pin in dagster and re-generates our protobuf api with protobuf 4 (which is mostly backwards compatible with protobuf3, see below) Two downsides of this: - we need to bump our min grpcio version to 1.44 since that's the first version that is protobuf3 and protobuf4 compatible, and our min version of protobuf a bit higher. This leaves our grpcio pin fairly narrow until we can get the upper bound removed once they fix the upstream issues. - it nukes our pyright typehints for the generated modules But users have been clamoring for us to remove this protobuf pin so I think it's worth it

gibsondan force-pushed the proto4 branch 10 times, most recently from 3b393f9 to 1f60d2d Compare February 22, 2023 17:18

gibsondan changed the title ~~test removing protobuf pin~~ Allow both protobuf 3 and 4 in dagster Feb 22, 2023

gibsondan mentioned this pull request Feb 22, 2023

Upgrade protobuf dependency to version 4.x #10627

Closed

gibsondan force-pushed the proto4 branch 4 times, most recently from b087711 to d1e8e6e Compare February 24, 2023 03:02

gibsondan changed the title ~~Allow both protobuf 3 and 4 in dagster~~ [For 1.2] Allow both protobuf 3 and 4 in dagster Feb 24, 2023

gibsondan requested review from smackesey, alangenfeld and schrockn and removed request for smackesey February 24, 2023 03:07

alangenfeld reviewed Feb 24, 2023

View reviewed changes

gibsondan mentioned this pull request Feb 24, 2023

Bump typing-extensions dep to >=4.4.0 #12529

Merged

gibsondan commented Mar 6, 2023

View reviewed changes

gibsondan force-pushed the proto4 branch from d1e8e6e to 0bd002f Compare March 6, 2023 12:16

gibsondan requested a review from alangenfeld March 6, 2023 12:21

gibsondan commented Mar 6, 2023

View reviewed changes

alangenfeld approved these changes Mar 6, 2023

View reviewed changes

gibsondan merged commit 589df01 into master Mar 6, 2023

gibsondan deleted the proto4 branch March 6, 2023 17:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[For 1.2] Allow both protobuf 3 and 4 in dagster #12466

[For 1.2] Allow both protobuf 3 and 4 in dagster #12466

gibsondan commented Feb 22, 2023 •

edited

Loading

vercel bot commented Feb 22, 2023 •

edited

Loading

alangenfeld left a comment

alangenfeld Feb 24, 2023

alangenfeld Mar 6, 2023

gibsondan Mar 6, 2023

alangenfeld Mar 6, 2023

gibsondan commented Feb 24, 2023

gibsondan Feb 24, 2023

gibsondan Mar 6, 2023

smackesey Mar 6, 2023

gibsondan Mar 6, 2023 •

edited

Loading

alangenfeld Mar 6, 2023

[For 1.2] Allow both protobuf 3 and 4 in dagster #12466

[For 1.2] Allow both protobuf 3 and 4 in dagster #12466

Conversation

gibsondan commented Feb 22, 2023 • edited Loading

vercel bot commented Feb 22, 2023 • edited Loading

alangenfeld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibsondan commented Feb 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibsondan Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibsondan commented Feb 22, 2023 •

edited

Loading

vercel bot commented Feb 22, 2023 •

edited

Loading

gibsondan Mar 6, 2023 •

edited

Loading