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

protos, python: use betterproto #315

Closed
wants to merge 2 commits into from

Conversation

woodruffw
Copy link
Contributor

WIP for now.

This replaces protobuf with betterproto, which:

  • Generates much nicer, more Pythonic code;
  • Is type-checked by default;
  • Provides an on-ramp for doing extended validation with Pydantic here (although that isn't presently possible, since betterproto doesn't support Pydantic V2 yet;
  • Is consistent with what Sigstore's protobuf-specs does, making the two more compatible.

The main downsides, in turn:

  • It's essentially a total rewrite, since the underlying protobuf runtime support library is changing from protobuf to betterproto. That means it'll be backwards-incompatible with previous versions;
  • Betterproto's 2.0 release is still in beta, although it has been stable in testing and is currently being used on protobuf-specs;
  • The high-level wrappers will need to be rewritten, along with their tests.

Closes #291.

Signed-off-by: William Woodruff <[email protected]>
@woodruffw
Copy link
Contributor Author

woodruffw commented Dec 19, 2023

(I haven't removed any of the old protobuf-based code yet. This PR just adds the new stuff on top.)

@marcelamelara
Copy link
Contributor

Thanks for getting this started! A couple thoughts.

It's essentially a total rewrite, since the underlying protobuf runtime support library is changing from protobuf to betterproto. That means it'll be backwards-incompatible with previous versions;

I'm not too concerned about this at the moment. Beyond in-toto's own (in-flight) transition to this library in our Python CLI, I don't know of other projects that currently depend on the current version of the Python library.

The high-level wrappers will need to be rewritten, along with their tests.

This is probably going to be the more cumbersome part. But if it's going to make writing future tests and using the library easier in the future, I think we can definitely motivate the re-implementation.

@woodruffw
Copy link
Contributor Author

I'm not too concerned about this at the moment. Beyond in-toto's own (in-flight) transition to this library in our Python CLI, I don't know of other projects that currently depend on the current version of the Python library.

Sounds good! The only other use I know of is sigstore-python's (also in-flight) use, which is currently ongoing here: #315

This is probably going to be the more cumbersome part. But if it's going to make writing future tests and using the library easier in the future, I think we can definitely motivate the re-implementation.

Makes sense -- I think this will absolutely make future testing and use easier, so I'll start the cumbersome bits now.

@marcelamelara
Copy link
Contributor

This is probably going to be the more cumbersome part. But if it's going to make writing future tests and using the library easier in the future, I think we can definitely motivate the re-implementation.

Makes sense -- I think this will absolutely make future testing and use easier, so I'll start the cumbersome bits now.

Thank you!

@woodruffw
Copy link
Contributor Author

Just to post a brief update: I've run into some snarls with betterproto's JSON ser/de. This unfortunately looks like a bug in betterproto itself; I'll do some more debugging tomorrow.

Signed-off-by: William Woodruff <[email protected]>
@woodruffw
Copy link
Contributor Author

Blocked on: danielgtaylor/python-betterproto#551

@marcelamelara
Copy link
Contributor

@woodruffw Pinging this PR. Is this still something you expect to get back to? Otherwise, we'll close this until we're ready to pick this back up.

@woodruffw
Copy link
Contributor Author

I'll get back to it eventually, but I probably won't have enough time in the next month or so. If that timeline is too far out, please close this!

@marcelamelara
Copy link
Contributor

No worries @woodruffw. We'll close this PR for now, but make sure the issue stays open!

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.

Python: Consider using betterproto for models?
2 participants