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

Preserve unknown proto2 fields and enums #29

Open
judah opened this issue Jul 13, 2016 · 2 comments
Open

Preserve unknown proto2 fields and enums #29

judah opened this issue Jul 13, 2016 · 2 comments

Comments

@judah
Copy link
Collaborator

judah commented Jul 13, 2016

We should preserve unknown fields and enums in proto2 messages.

From the docs:
https://developers.google.com/protocol-buffers/docs/proto

Similarly, messages created by your new code can be parsed by your old code: old binaries simply ignore the new field when parsing. However, the unknown fields are not discarded, and if the message is later serialized, the unknown fields are serialized along with it – so if the message is passed on to new code, the new fields are still available.

Also, enums with unknown values should be treated like unknown fields and preserved for reserialization. (Note that this behavior is proto2-only; #28 describes the desired behavior for proto3.)

@judah
Copy link
Collaborator Author

judah commented Jul 10, 2017

Update: as described in protocolbuffers/protobuf#272, the specification is changing to preserve unknown fields in proto3 as well as proto2. See also:
https://docs.google.com/document/d/1KMRX-G91Aa-Y2FkEaHeeviLRRNblgIahbsk4wA14gRk/edit#heading=h.w8dtggryroj4

When we implement this behavior, we should follow the new spec.

@judah
Copy link
Collaborator Author

judah commented Sep 1, 2017

This has been mostly done for unknown fields (but not enums), in #129.

There are a couple remaining edge cases:

  • Printing unknown groups is more unstructured than it could be
  • The discardUnknownFields function doesn't act recursively (unlike other languages)

blackgnezdo added a commit that referenced this issue Aug 17, 2018
Includes temporary blacklisting for a couple of ops that will be
supported once my fix lands in the main tensorflow repo.
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

No branches or pull requests

1 participant