feat: Add Explanations to service APIs by vinnysenthil · Pull Request #216 · googleapis/python-aiplatform

vinnysenthil · 2021-02-09T18:27:48Z

Lifts the GAPIC types ExplanationMetadata and ExplanationParameters to the aiplatform namespace
Adds the ability to pass the two fields in ExplanationSpec to the following methods
- Model.batch_predict()
- Model.deploy()
- Model.upload()
- Endpoint.deploy()
- BatchPredictionJob.create()
Add the method Endpoint.explain() to get predictions with explanations on deployed models
Unit tests for all the above changes

Fixes b/174502675 and part 1 of b/172828587 🦕

ivanmkc

I haven't used explanations before but in general LGTM

sasha-gitg

Added a few comments. Thanks!

sasha-gitg · 2021-02-16T15:04:39Z

+from google.cloud.aiplatform_v1beta1.types.explanation_metadata import (
+    ExplanationMetadata,
+)
+from google.cloud.aiplatform_v1beta1.types.explanation import ExplanationParameters


It makes sense to elevate these protos to the aiplatform namespace but ExplanationParameters relies on additional protos which are not elevated, requiring the user import from multiple modules to build the spec. Perhaps all of the required explanation protos should be encapsulated in the aiplatform.explain namespace.

Sounds good, will elevate all Explanation protos to aiplatform.explain

Actually only including protos that are used as inputs, and only raising ExplanationMetadata subclasses that are two levels deep. Lmk if you think we should include outputs too.

sasha-gitg

LGTM. Left a two more comments.

vinnysenthil · 2021-02-18T19:54:15Z

All requested changes from MB SDK Dev addressed. Blocking merge until receiving LGTM from XAI Eng.

besimav

LGTM overall.

besimav · 2021-02-23T22:43:28Z

+Encoding = ExplanationMetadata.InputMetadata.Encoding
+FeatureValueDomain = ExplanationMetadata.InputMetadata.FeatureValueDomain
+Visualization = ExplanationMetadata.InputMetadata.Visualization
+


a bit orthogonal question: why don't we have Modality class?

It appears that modality is a string field in InputMetadata according to the protos. Should it be an enum?

I was looking at this proto. Do you know why it is an Enum in the DB one and not in the other by any chance? Anyhow, lgtm.

Interesting, I raised this with jianan@ and he explained they lean towards string over enums in the API protos to avoid having to make frequent changes to those files while the DB protos are more strongly typed. If it improves usability, we could manually add the enum in the SDK but it would have to be kept in sync.

nanchenchen

Thanks for working on this! It looks good to me 👍
I only have a minor comment. Should be good to go after addressing it.

nanchenchen · 2021-02-26T14:56:56Z

+        if explanation_metadata or explanation_parameters:
+            explanation_spec = gca_endpoint.explanation.ExplanationSpec()
+            explanation_spec.metadata = explanation_metadata
+            explanation_spec.parameters = explanation_parameters
+            deployed_model.explanation_spec = explanation_spec
+


Since explanation_metadata and explanation_parameters must pass together, shouldn't this be an and instead of or?

The thought behind this was allowing the service to throw the exception about them being passed together instead of creating two sets of validation. I confirmed that an error response is returned service-side right away if only one is passed.

We inform the user in the docstring. Do you think we should add validation in the library as well?

python-aiplatform/google/cloud/aiplatform/models.py

Lines 480 to 481 in b5000c2

Both `explanation_metadata` and `explanation_parameters` must be

passed together when used. For more details, see

I think telling users what will fail as early as possible will be better. We don't necessary need full validation (though it is good to have if it is low cost), but having some basic sanity check can help prevent user frustration (especially since all the operations here are async and have some wait time).

Also, I noticed that there other similar logics are using and (e.g., Ln 1287), so I thought it was unintentional :)

Good point, I've added the local validation for ensuring they are passed together.

Expose Explanations in service APIs

e1ca3d9

vinnysenthil requested review from morgandu, sasha-gitg and sirtorry February 9, 2021 18:27

google-cla Bot added the cla: yes This human has signed the Contributor License Agreement. label Feb 9, 2021

ivanmkc approved these changes Feb 10, 2021

View reviewed changes

sasha-gitg requested changes Feb 16, 2021

View reviewed changes

vinnysenthil added 4 commits February 16, 2021 13:19

Merge branch 'dev' into xai-api-only

5d0ea8b

Add aiplatform.explain, fix docstrings, expose arg

4dd02f6

Merge branch 'dev' into xai-api-only

b33e571

Drop trailing space

8ff7010

vinnysenthil requested review from a team and sasha-gitg February 17, 2021 00:53

sasha-gitg approved these changes Feb 17, 2021

View reviewed changes

Comment thread google/cloud/aiplatform/jobs.py Outdated

Comment thread google/cloud/aiplatform/models.py

Change BP XAI default to False, allow user to send partial explain spec

b5000c2

vinnysenthil added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 17, 2021

vinnysenthil mentioned this pull request Feb 18, 2021

feat: Proof of concept - Merge XAI SDK + add required fields for Explanation in DeployedModel #208

Closed

vinnysenthil requested a review from besimav February 23, 2021 22:06

besimav reviewed Feb 23, 2021

View reviewed changes

vinnysenthil requested a review from nanchenchen February 25, 2021 18:15

nanchenchen approved these changes Feb 26, 2021

View reviewed changes

vinnysenthil added 2 commits March 3, 2021 14:52

Add local validation for passing XAI params and metadata together

3f36294

Merge branch 'dev' into xai-api-only

3f1df68

vinnysenthil removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 3, 2021

vinnysenthil merged commit 350ffd5 into googleapis:dev Mar 4, 2021

	Both `explanation_metadata` and `explanation_parameters` must be
	passed together when used. For more details, see

Conversation

vinnysenthil commented Feb 9, 2021

Uh oh!

ivanmkc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sasha-gitg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sasha-gitg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vinnysenthil commented Feb 18, 2021

Uh oh!

besimav left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nanchenchen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ivanmkc left a comment •

edited

Loading