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

feat: add webhook to model version deployment & undeployment #601

Merged
merged 6 commits into from
Aug 19, 2024

Conversation

bthari
Copy link
Contributor

@bthari bthari commented Aug 12, 2024

Description

When deploying or undeploying model endpoint, other entity might want to get a trigger to automate their process or for Merlin other process. This PR add webhooks call (based on the webhook from MLP), so on model version deployment/undeployment it will call the configured webhooks.

Modifications

Main Changes:

Added webhooks call on

  • Model version pre-deployment: will ignore the success response (for this version) and will stop/fail the deployment version if any async webhook fail
  • Model version post-deployment: will ignore error, only log the error if any occur during the call
  • Model version post-undeployment: will ignore error, only log the error if any occur during the call

Request payload to webhook:

  • Request:
    • event_type: name of event which triggers the webhook
    • versionEndpoint: object version endpoint

Side effect changes:

  • With MLP update in go.mod, the assert function is also updated. Previously, the assert.InEpsilon can pass when item in actual slice is in expected slice, even though the expected slice might have more items; now the testify/assert will check the two slices length first (ref) -> Added some changes to fix the unit test in TestToFloat64List

Tests

Checklist

  • Added PR label
  • Added unit test, integration, and/or e2e tests
  • Tested locally
  • Updated documentation
  • Update Swagger spec if the PR introduce API changes
  • Regenerated Golang and Python client if the PR introduces API changes

Release Notes

add configurable webhook call in endpoint deployment and undeployment

Copy link

codecov bot commented Aug 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.71%. Comparing base (cbad4a2) to head (e6f1bd1).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #601      +/-   ##
==========================================
+ Coverage   60.65%   60.71%   +0.06%     
==========================================
  Files         274      274              
  Lines       22116    22150      +34     
==========================================
+ Hits        13415    13449      +34     
+ Misses       7844     7837       -7     
- Partials      857      864       +7     
Flag Coverage Δ
api-test 58.68% <ø> (+0.05%) ⬆️
sdk-test-3.10 75.51% <ø> (+0.05%) ⬆️
sdk-test-3.8 75.49% <ø> (+0.05%) ⬆️
sdk-test-3.9 75.49% <ø> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bthari bthari added the enhancement New feature or request label Aug 13, 2024
@bthari bthari marked this pull request as ready for review August 13, 2024 06:41
@bthari bthari self-assigned this Aug 14, 2024
api/service/version_endpoint_service.go Outdated Show resolved Hide resolved
api/service/version_endpoint_service.go Outdated Show resolved Hide resolved
Copy link
Contributor

@tiopramayudi tiopramayudi left a comment

Choose a reason for hiding this comment

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

LGTM ! Thanks @bthari

@bthari bthari merged commit da2803a into main Aug 19, 2024
35 checks passed
@bthari bthari deleted the feat-webhook-model-version-deployment branch August 19, 2024 09:28
@bthari bthari mentioned this pull request Sep 25, 2024
6 tasks
bthari added a commit that referenced this pull request Oct 7, 2024
# Description
Adding documentation about the webhook created in this PR #601; how to
add the config, what event is available, and the information about the
payload.

# Modifications

# Tests

# Checklist
- [x] Added PR label
- [ ] Added unit test, integration, and/or e2e tests
- [ ] Tested locally
- [x] Updated documentation
- [ ] Update Swagger spec if the PR introduce API changes
- [ ] Regenerated Golang and Python client if the PR introduces API
changes

# Release Notes

```release-note
NONE
```
@bthari bthari mentioned this pull request Oct 8, 2024
6 tasks
bthari added a commit that referenced this pull request Oct 15, 2024
# Description
Previously on #601 I added webhook for model version endpoint related
event, and in here the event will be expanded into a model, model
endpoint, model version related event, as we also want to have an action
(from other service) to be triggered if these events happen.

# Modifications
<!-- Summarize the key code changes. -->

- created another package for webhook interface 
- add event for:
  - model created
  - model endpoint created/updated/deleted
  - model version created/updated/deleted
- change previous event name of `on-model-version-*` to
`on-version-endpoint-*`


# Tests

# Checklist
- [x] Added PR label
- [x] Added unit test, integration, and/or e2e tests
- [x] Tested locally
- [ ] Updated documentation
- [ ] Update Swagger spec if the PR introduce API changes
- [ ] Regenerated Golang and Python client if the PR introduces API
changes

# Notes for Reviewer
The version of MLP used here has a validation bug (which is updated on
[MLP#117](caraml-dev/mlp#117)). The code could
still work with workaround (e.g. set `FinalResponse: true` in one async
webhook if user use _all_ async webhook, but it will be confusing for
user since async webhook response is expected to not be used anywhere),
~~so preferably to merge this PR after updating the MLP version as
dependencies.~~ (MLP version will be updated with the s3 PR)

# Release Notes
<!--
Does this PR introduce a user-facing change?
If no, just write "NONE" in the release-note block below.
If yes, a release note is required. Enter your extended release note in
the block below.
If the PR requires additional action from users switching to the new
release, include the string "action required".

For more information about release notes, see kubernetes' guide here:
http://git.k8s.io/community/contributors/guide/release-notes.md
-->

```release-note
add webhook event call if there's changes on model, model endpoint, model version
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants