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

Protobuf SerDes support #402

Merged
merged 1 commit into from
Aug 1, 2024
Merged

Protobuf SerDes support #402

merged 1 commit into from
Aug 1, 2024

Conversation

quentin-quix
Copy link
Contributor

No description provided.

@quentin-quix quentin-quix force-pushed the quent/protobuff branch 4 times, most recently from b09fe6d to 0942db8 Compare July 17, 2024 11:30
@quentin-quix quentin-quix marked this pull request as ready for review July 17, 2024 11:57
@quentin-quix quentin-quix changed the title Draft: Protobuf SerDes support Protobuf SerDes support Jul 17, 2024
Copy link
Collaborator

@daniil-quix daniil-quix left a comment

Choose a reason for hiding this comment

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

Hey @quentin-quix, it looks good overall, with only a few minor comments 👍

Question: How well does it work with more complex Protobuf types like Enums? E.g., Enum can be deserialized as enum.Enum and as an int.
Do we cover that?

Also, please remember to add the info about Protobuf to the docs (see docs/advanced/serialization.md).

pyproject.toml Outdated Show resolved Hide resolved
quixstreams/models/serializers/protobuf.py Outdated Show resolved Hide resolved
@quentin-quix
Copy link
Contributor Author

Question: How well does it work with more complex Protobuf types like Enums? E.g., Enum can be deserialized as enum.Enum and as an int.
Do we cover that?

There is an argument for it that we should expose, use_integers_for_enums.

@quentin-quix quentin-quix force-pushed the quent/protobuff branch 4 times, most recently from 6e942bd to 357e373 Compare July 23, 2024 14:40
Copy link
Contributor

@gwaramadze gwaramadze left a comment

Choose a reason for hiding this comment

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

I think new serdes classes need to be imported and added to SERIALIZERS and DESERIALIZERS maps in quixstreams.models.serializers.__init__

@quentin-quix
Copy link
Contributor Author

I think new serdes classes need to be imported and added to SERIALIZERS and DESERIALIZERS maps in quixstreams.models.serializers.__init__

We don't want to do it for protobuf since it needs an extra import. Importing it in quixstreams.models.serializers.__init__ would raise an ImportError due to the possibly missing dependencies. We could catch the error and skip it but iirc we decided against it

@quentin-quix quentin-quix force-pushed the quent/protobuff branch 3 times, most recently from 018cbfe to fde4dc5 Compare July 31, 2024 16:08
@daniil-quix daniil-quix merged commit 609c24a into main Aug 1, 2024
3 checks passed
@daniil-quix daniil-quix deleted the quent/protobuff branch August 1, 2024 11:47
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.

3 participants