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

feat/river schema v2 compatibility #106

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

Conversation

blast-hardcheese
Copy link
Contributor

@blast-hardcheese blast-hardcheese commented Nov 6, 2024

Why

River Schema v2 compatibility so we can start getting rid of the compatibility layer.

This PR is just the first, it does not introduce the half-close semantics yet.

What changed

  • init is now required for all method types, input is now optional. This simplifies rpc and stream codegen.
  • Fixed some bugs where init was being used in input position and vice versa.
  • Swapping f"'{foo}'" encoding for f"{repr(foo)}", giving greater safety post-generation (to avoid situations where f"'{None}'" gets rendered as "None". f"{repr(None)}" would render as None which would fail typechecks.)
  • Simplify optional return
  • Swapping List and Dict to list and dict
  • Swapping encode_... lambdas for def, to placate both line length as well as lint suggestions (and increase readability)

Test plan

Manually ran codegen against v2 generated schemas, everything typechecks.

@blast-hardcheese blast-hardcheese requested a review from a team as a code owner November 6, 2024 06:40
@blast-hardcheese blast-hardcheese requested review from masad-frost and removed request for a team November 6, 2024 06:40
@blast-hardcheese blast-hardcheese added the enhancement New feature or request label Nov 6, 2024
@blast-hardcheese blast-hardcheese force-pushed the dstewart/feat/river-v2-codegen branch 2 times, most recently from a59a4df to dadffb8 Compare November 6, 2024 07:31
Copy link
Member

@masad-frost masad-frost left a comment

Choose a reason for hiding this comment

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

Do you wanna set up a different base branch in case we need to send a patch against main?

if x['{discriminator_name}']
== '{discriminator_value}'
if x[{repr(discriminator_name)}]
== {repr(discriminator_value)}
Copy link
Member

Choose a reason for hiding this comment

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

huh, how did this even work lol

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 equivalent in the happy path but repr will surface bugs in the not happy path

Comment on lines +86 to +89
init: InitType,
request: Optional[AsyncIterable[RequestType]],
init_serializer: Callable[[InitType], Any],
request_serializer: Optional[Callable[[RequestType], Any]],
Copy link
Member

Choose a reason for hiding this comment

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

Ideally the type is if request exists then request_serializer should not be optional.

Copy link
Contributor Author

@blast-hardcheese blast-hardcheese Nov 6, 2024

Choose a reason for hiding this comment

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

Maybe it should be request: Optional[ tuple[ {request_type}, {request_serializer_type} ] ]

procedure_name=procedure_name,
payload=init_serializer(init),
)
first_message = False
Copy link
Member

Choose a reason for hiding this comment

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

first_message should be no longer needed since we always send a STREAM_OPEN_BIT and we can remove the bit from send_close_stream

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on how the code worked previously, if we had an exception during sending the first message, then STREAM_OPEN_BIT was sent in the send_close_stream -- was that not intentional?

Comment on lines +115 to +120
control_flags = 0
await self.send_message(
stream_id=stream_id,
service_name=service_name,
procedure_name=procedure_name,
control_flags=control_flags,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
control_flags = 0
await self.send_message(
stream_id=stream_id,
service_name=service_name,
procedure_name=procedure_name,
control_flags=control_flags,
await self.send_message(
stream_id=stream_id,
service_name=service_name,
procedure_name=procedure_name,
control_flags=0,

'{name}',
input,
{render_input_method},
return await self.client.send_rpc(
Copy link
Member

Choose a reason for hiding this comment

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

all this stuff is hard to review (and I'm sure iterate on), we should set up some simple tests. Snapshot testing is probably good enough https://pypi.org/project/pytest-snapshot/.

I did this for river js replit/river#261 using https://vitest.dev/guide/snapshot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The best way I have found to iterate on this is to just run the codegen against different production schemas and verify that the set of changes is what I expect. I'm happy to wait until after I land the babel changes before merging this.

The codegen currently isn't compositional enough to make snapshot testing manageable. The more I work with it though I've been trying to come up with strategies to make it less imperative without increasing cognitive load too much, I'll land snapshot testing then.

]
# Non-oneof fields.
oneofs: DefaultDict[int, List[descriptor_pb2.FieldDescriptorProto]] = (
oneofs: DefaultDict[int, list[descriptor_pb2.FieldDescriptorProto]] = (
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference?

I wonder if it's like the stupid typescript thing where String (object) is different from string (literal/primitive) 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Python 3.8 and below did not permit the builtin types to be subscripted, typing.List and typing.Dict were added as future-compatible shims during the migration. Python 3.8 is EOL as of the end of Spooky Season, so now we can 🚀.

@blast-hardcheese blast-hardcheese force-pushed the dstewart/feat/river-v2-codegen branch from dadffb8 to 4c0e071 Compare November 6, 2024 21:03
@blast-hardcheese
Copy link
Contributor Author

Do you wanna set up a different base branch in case we need to send a patch against main?

If we need to patch, we should do feature patches off previous release tags. Since we're reserving 1.x for GA, I can follow the 0.200.0 strategy in this repo for schema-v2 releases.

Doing that though would suggest that we do at least a halfway decent pass at the v2 server codegen as well though. Given what we discussed supporting v1/v2 using a naive shim doesn't seem too bad, probably can get that done this week.

Wdyt?

@blast-hardcheese blast-hardcheese force-pushed the dstewart/feat/river-v2-codegen branch from 4c0e071 to b71a809 Compare November 7, 2024 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants