feat(gRPC): Add proto conversion utilities #420
Conversation
- Fix JSON serialization error for custom objects in metadata by adding _make_json_serializable helper method that converts non-serializable objects to strings recursively - Fix precision loss for large integers (>2^53-1) by storing them as strings and converting back to int when deserializing - Update type hints to use union syntax instead of tuple for isinstance - All tests now pass including test_metadata_with_custom_objects and test_metadata_edge_cases
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @youngchannelforyou, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request enhances metadata serialization and deserialization by introducing robust handling for custom Python objects and large integers. It addresses previous limitations where custom objects caused JSON serialization errors and large integers lost precision. The changes involve new utility methods to convert diverse Python types to and from protobuf struct_pb2.Value objects, ensuring data integrity and broader compatibility for metadata.
Highlights
- Enhanced Metadata Serialization: Implemented _convert_value_to_proto to support a wider range of Python data types (including None, booleans, floats, dictionaries, lists, and tuples) when converting metadata to protobuf struct_pb2.Value.
- Custom Object Serialization: Introduced _make_json_serializable to recursively convert non-JSON-serializable custom objects within metadata to their string representations, preventing serialization errors.
- Large Integer Precision Preservation: Ensured large integers (absolute value greater than 2^53 - 1) are serialized as strings to prevent precision loss and correctly deserialized back into integers.
- Comprehensive Deserialization: Added _convert_proto_to_value to accurately convert protobuf struct_pb2.Value objects back into their original Python data types, including the re-conversion of string-encoded large integers.
- Updated Type Hinting: Modernized type hints to use the union operator (|) for improved readability and consistency.
- Expanded Test Coverage: Added new unit tests (test_metadata_conversion, test_metadata_with_custom_objects, test_metadata_edge_cases) to validate the new serialization and deserialization logic across various data types and edge cases.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review | Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary | Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help | Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces robust metadata serialization by handling custom objects and large integers, preventing JSON serialization errors and precision loss. The changes are well-tested. My review includes a suggestion to refactor the serialization logic for better maintainability and to address a potential issue where numeric strings could be unintentionally converted to integers. I've also suggested an additional test case to cover this edge case.
- Added _MAX_SAFE_INTEGER_DIGITS constant to improve code readability - Replaced hardcoded value 15 with the named constant in metadata conversion
…conversion - Remove _make_json_serializable method entirely - Refactor _convert_value_to_proto to handle dictionaries and lists recursively - Fix null value handling to use struct_pb2.NULL_VALUE - Improve code efficiency and maintainability - All tests passing
- Add _make_dict_serializable helper method to recursively convert dict values - Convert custom objects to strings using str() for JSON serialization - Fix test_metadata_with_custom_objects test failure - Improve code style with modern Python union syntax (| operator)
- Add test case for numeric strings (e.g., zip codes, numeric IDs) in metadata edge cases - Fix mypy type error in _convert_value_to_proto for null_value assignment - Use proper struct_pb2.NullValue.NULL_VALUE instead of integer 0 - Ensure numeric strings remain as strings after roundtrip conversion
| I am not sure we should be trying to infer integers from strings during this translation. It potentially causes incompatibility between the two representations when translating back and forth. If someone is using a string to represent an integer in their struct they should be responsible for parsing this out where needed and it shouldn't be in the translation layer, imho It seems that you really want a preprocessor on the dict provided in (dict->Struct) and a post processor on the dict produced in (Struct->dict) to apply some normalizations. I think that should be the responsibility of the creators/consumers and not in the proto translation layer itself. Can you provide these as stand alone utility methods? |
…nctions - Remove complex automatic type inference from translation layer - Replace custom _convert_value_to_proto/_convert_proto_to_value with simple dict_to_struct and json_format.MessageToDict - Add standalone utility functions for preprocessing/post-processing: - make_dict_serializable: converts non-serializable values to strings - normalize_large_integers_to_strings: handles JavaScript MAX_SAFE_INTEGER compatibility - parse_string_integers_in_dict: converts large integer strings back to integers - Update tests to use new utility functions and add comprehensive test coverage - Ensure bidirectional conversion consistency and user control over normalization This addresses concerns about automatic string-to-integer inference causing incompatibility between representations. The translation layer is now simple and predictable, with normalization being the responsibility of consumers.
| @pstephengoogle Thank you for the feedback! I've refactored the proto_utils to remove automatic type inference from the translation layer and provided standalone utility functions for preprocessing/post-processing instead. |
There was a problem hiding this comment.
Minor efficiency suggestions. @youngchannelforyou Can you also update your PR message to reflect the new structure?
holtskinner changed the title Co-authored-by: Holt Skinner <13262395+holtskinner@users.noreply.github.com>
Co-authored-by: Holt Skinner <13262395+holtskinner@users.noreply.github.com>
Co-authored-by: Holt Skinner <13262395+holtskinner@users.noreply.github.com>
…ub.com/youngchannelforyou/a2a-python into pr/youngchannelforyou/420
| @youngchannelforyou Just update the PR message with the new structure and we're good to merge |
youngchannelforyou changed the title | Thank you so much for the code review that inspires novice engineers. I'll come back with better code. |
holtskinner changed the title 
3 participants
BEGIN_COMMIT_OVERRIDE
feat(gRPC): Add proto conversion utilities
Release-As: 0.3.6
END_COMMIT_OVERRIDE
make_dict_serializableto prepare dictionaries for proto conversion.normalize_large_integers_to_stringsto convert large integers to strings, preventing precision loss in JS clients.parse_string_integers_in_dictto convert the integer strings back toint.