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

Add support for pluggable explainer runtimes #3564

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

TimKleinloog
Copy link
Contributor

What this PR does / why we need it:
For additional context see #3398

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Implements #3398
New clean PR resulting from #3418

Type of changes

  • New feature (non-breaking change which adds functionality)

Feature/Issue validation/testing:

Please describe the tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.

  • Run make deploy-dev and deploy the new explainer sample that uses an explainer runtime docs/samples/v1beta1/explanation/deeploy-shap/deeploy-sample.yaml

Special notes for your reviewer:

  1. Some changes include naming decisions: ModelSpec is reused for ExplainerSpec, how to both use PredictorExtensionSpec and ExplainerExtensionSpec in ModelSpec?

Checklist:

  • Have you added unit/e2e tests that prove your fix is effective or that this feature works?
  • Has code been commented, particularly in hard-to-understand areas?
  • Have you made corresponding changes to the documentation?

Release note:

You can now configure a model in the ExplainerSpec to plug any arbitrary framework

@TimKleinloog TimKleinloog reopened this Apr 3, 2024
Copy link

oss-prow-bot bot commented Apr 3, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: TimKleinloog
Once this PR has been reviewed and has the lgtm label, please assign rachitchauhan43 for approval by writing /assign @rachitchauhan43 in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@TimKleinloog
Copy link
Contributor Author

TimKleinloog commented Apr 10, 2024

@terrytangyuan I started this clean PR here after #3443 was merged. The tests look to succeed now, is it already possible to merge this?

@@ -26,6 +26,8 @@ import (
type ExplainerSpec struct {
// Spec for ART explainer
ART *ARTExplainerSpec `json:"art,omitempty"`
// Explainer spec for any arbitrary framework.
Model *ModelSpec `json:"model,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

This is currently defined in predictor.go. Perhaps we should extract that out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually defined in model.go which I renamed (was predictor_model.go) so it is more logical that is used in both explainer.go and predictor.go. In model.go the PredictorExtensionSpec is used which is defined in predictor.go do you think we should extract that out?

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

@TimKleinloog Can you limit the scope of this PR to only include the new feature? New runtimes can be added via a separate PR as an example for now.

@TimKleinloog
Copy link
Contributor Author

@TimKleinloog Can you limit the scope of this PR to only include the new feature? New runtimes can be added via a separate PR as an example for now.

I will open a new PR and remove the explainer runtime example from the scope of this PR 👌

Signed-off-by: Tim Kleinloog <tkleinloog@deeploy.ml>
@TimKleinloog TimKleinloog force-pushed the pluggable-explainer-runtimes branch from dbf83e2 to 9053725 Compare May 6, 2024 12:17
Signed-off-by: Tim Kleinloog <tkleinloog@deeploy.ml>
Signed-off-by: Tim Kleinloog <tkleinloog@deeploy.ml>
@yuzisun
Copy link
Member

yuzisun commented May 19, 2024

@TimKleinloog Can you help resolve the conflicts?

Signed-off-by: Tim Kleinloog <tkleinloog@deeploy.ml>
Signed-off-by: Tim Kleinloog <tkleinloog@deeploy.ml>
@TimKleinloog
Copy link
Contributor Author

@TimKleinloog Can you help resolve the conflicts?

Done, hope this will be one of the last conflicts to resolve :)

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.

None yet

3 participants