Skip to content

Conversation

ji-yaqi
Copy link
Contributor

@ji-yaqi ji-yaqi commented Sep 1, 2021

Add a explanationMetadata Object for re-use.

@ji-yaqi ji-yaqi requested review from a team as code owners September 1, 2021 04:55
@product-auto-label product-auto-label bot added the api: aiplatform Issues related to the AI Platform API. label Sep 1, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 1, 2021
Copy link
Member

@sasha-gitg sasha-gitg left a comment

Choose a reason for hiding this comment

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

Changing the return type on get_metadata is a breaking change. Preference to avoid it if possible.

Another potential solution is expanding the explain parameters to accept a Union of Dict and Explain protos and handling the conversion underneath.

Proto plus seems to be able to accept dictionaries of representation at construction time: https://github.com/googleapis/proto-plus-python/blob/cfd5b6caca3fa9add89d8c69ea620505dd90dd7c/docs/messages.rst#serialization

What is the exception that is thrown if the dictionary representation is passed in?

@ji-yaqi
Copy link
Contributor Author

ji-yaqi commented Sep 2, 2021

Changing the return type on get_metadata is a breaking change. Preference to avoid it if possible.

Another potential solution is expanding the explain parameters to accept a Union of Dict and Explain protos and handling the conversion underneath.

Proto plus seems to be able to accept dictionaries of representation at construction time: https://github.com/googleapis/proto-plus-python/blob/cfd5b6caca3fa9add89d8c69ea620505dd90dd7c/docs/messages.rst#serialization

What is the exception that is thrown if the dictionary representation is passed in?

I think we are not changing the return type on get_metadata, but adding an additional get_metadata_object to return the real object. It's mainly type changes.

@sasha-gitg
Copy link
Member

Got it. Disregard the breaking change statement.

Would like to follow up on the exception thrown when passing the dictionary instead of the proto.

@ji-yaqi
Copy link
Contributor Author

ji-yaqi commented Sep 3, 2021

Got it. Disregard the breaking change statement.

Would like to follow up on the exception thrown when passing the dictionary instead of the proto.

I got the following: Parameter to MergeFrom() must be instance of same class: expected google.cloud.aiplatform.v1beta1.ExplanationMetadata.InputMetadata got dict.

@ji-yaqi ji-yaqi requested a review from sasha-gitg September 7, 2021 21:52
"""Returns the current metadata as a dictionary."""

@abc.abstractmethod
def get_metadata_object(self):
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this can be informative if further qualified as get_metadata_protobuf or get_metadata_pb.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, changed to get_metadata_protobuf

@ji-yaqi ji-yaqi changed the title feat: add explanation metadata object for reuse feat: add explanation metadata get_metadata_protobuf for reuse Sep 8, 2021
@andrewferlitsch
Copy link
Contributor

/LGTM

@ji-yaqi ji-yaqi merged commit efb6d18 into googleapis:main Sep 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: aiplatform Issues related to the AI Platform API. cla: yes This human has signed the Contributor License Agreement.

4 participants