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

Import Google's protobuf matchers via Nucleus. #32

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SanjayVas
Copy link
Member

@SanjayVas SanjayVas commented Jun 25, 2022

Google's protobuf matchers are open-sourced in the Nucleus project. This imports those protobuf matchers by replacing the dependency on TensorFlow with glog.

See google/googletest#1761


This change is Reviewable

@SanjayVas SanjayVas force-pushed the sanjayvas-proto-matchers branch from 4af8dd6 to 18276c0 Compare June 25, 2022 00:48
Google's protobuf matchers are open-sourced in the Nucleus project. This imports those protobuf matchers by replacing the dependency on TensorFlow with glog.
@SanjayVas SanjayVas force-pushed the sanjayvas-proto-matchers branch from 18276c0 to 1062b2d Compare June 25, 2022 01:02
Copy link
Contributor

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SanjayVas)


build/nucleus/repo.bzl line 27 at r1 (raw file):

        sha256 = "326907b1ae0a3419806b52ff1bd80502d842248edc7986a89fba9fc4ed37ff0b",
        strip_prefix = "nucleus-0.6.0",
        patches = ["@wfa_common_cpp//build/nucleus:testing.patch"],

isn't this going to be hard to maintain? Not sure what I think about going this route. What's the shortcoming in our current protobuf matchers that we need to use a patched nucleus intead?

Copy link
Member Author

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SanjayVas)


build/nucleus/repo.bzl line 27 at r1 (raw file):

What's the shortcoming in our current protobuf matchers

At a glance:

  • Supporting text format protos in addition to message instances.
  • Feature parity with ProtoTruth:
    • Ignoring specific fields.
    • Partial matching (similar to ProtoTruth's comparingExpectedFieldsOnly)
    • Approximate matching for floating point values.

isn't this going to be hard to maintain?

Protobuf matchers in Nucleus are have not changed in years. Last update was in 2018.

That said, I think I found a "clean" version in https://github.com/google-research/tf-opt/tree/master/tf_opt/open_source that's both more up-to-date and does not depend on TensorFlow. We may be able to pull that in without patching. Let me investigate.

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.

2 participants