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

fix(sdk): Propagate transformer ID during re-deployment #497

Merged
merged 7 commits into from
Nov 29, 2023

Conversation

krithika369
Copy link
Collaborator

@krithika369 krithika369 commented Nov 28, 2023

What this PR does / why we need it:

This PR exposes the Transformer id to the SDK methods so that, in the event of a model redeployment, the transformer's ID can be re-used to trigger an update.

Which issue(s) this PR fixes:

Model redeployment capabilities were added to the SDK in #455. With this, however, transformer updates were not being done correctly and instead, a new transformer entity would be attempted to be created with every version endpoint update (re-deployment). The re-deployment operation would eventually fail with an error like below, leaving the endpoint stuck in the "pending" state:

unable to update endpoint status for model: abcd, version: 2, reason: ERROR: duplicate key value violates unique constraint "transformers_version_endpoint_id_key" (SQLSTATE 23505)

This PR addresses this issue by exposing the transformer's id to the SDK and copying the id to the new transformer specs, during version endpoint updates.

Summary of changes

  • swagger.yaml - Update Transformer specs to include id
  • api/client/, python/sdk/client/ - Re-generate client code
  • python/sdk/merlin/endpoint.py - Add transformer property to VersionEndpoint
  • python/sdk/merlin/model.py
    • Propagate transformer id during re-deployment
    • Re-get the endpoint after a deployment so that its status would've updated to "pending". When updating an endpoint (PUT), the status change to "pending" is not immediate and may not be reflected in the response from the update call.
  • python/sdk/merlin/transformer.py - Add id property to the Transformer class
  • python/sdk/test/integration_test.py - Expand the re-deployment test case to cover the id check for transformer. In the test case, the Tensorflow model (which requires a specific input) is replaced with a custom model so that the echo transformer can be used alongside.
  • python/sdk/test/model_test.py - Update mock responses, owning to the change in python/sdk/merlin/model.py to re-get the version endpoint after deployment.

Does this PR introduce a user-facing change?:

Fix: Update transformer specs correctly when re-deploying a model through the SDK

Checklist

  • 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 introduce API changes

@krithika369 krithika369 marked this pull request as draft November 28, 2023 02:22
@ghost
Copy link

ghost commented Nov 28, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@krithika369 krithika369 marked this pull request as ready for review November 28, 2023 12:02
@ariefrahmansyah
Copy link
Contributor

LGTM, thanks @krithika369 !

@krithika369 krithika369 merged commit 67b0583 into main Nov 29, 2023
36 checks passed
@krithika369 krithika369 deleted the fix_transformer_redeploy branch November 29, 2023 03:21
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