-
Notifications
You must be signed in to change notification settings - Fork 381
feat: Added sync argument in Matching Engine IndexEndpoint deploy_index #5305
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
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
CLA Signed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I pointed to an existing decorator in our codebase that should be used to implement the desired behavior. This change also needs a test for the sync parameter behavior, something analogous to
def test_delete_index_endpoint(self, delete_index_endpoint_mock, sync): |
google/cloud/aiplatform/matching_engine/matching_engine_index_endpoint.py
Outdated
Show resolved
Hide resolved
google/cloud/aiplatform/matching_engine/matching_engine_index_endpoint.py
Show resolved
Hide resolved
google/cloud/aiplatform/matching_engine/matching_engine_index_endpoint.py
Outdated
Show resolved
Hide resolved
@ericgribkoff Thank you for your response. I have added the requested changes, including the updated test cases for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for addressing the earlier comments!
@matthew29tang Lingyin rerouted the review to me. Changes look good and approved from Vector Search team's side, but I don't have write access to this repository. |
I will get this merged in then, thanks for the review! |
-- 7135692 by Kenny Stryker <nggkenny@gmail.com>: feat(matching-engine): add sync argument to deploy_index -- e0eb35d by Kenny Stryker <nggkenny@gmail.com>: feat: Updated explicit sync to existing decorator optional_sync -- af64657 by Kenny Stryker <nggkenny@gmail.com>: fix: Fixed return type for deploy_index and added test for sync values -- 4a1edb1 by Kenny Stryker <nggkenny@gmail.com>: test: Added test cases for sync values in automatic config deploy index -- b0d4e47 by Kenny Stryker <nggkenny@gmail.com>: chore: Updated docstring for helper method _deploy_index COPYBARA_INTEGRATE_REVIEW=#5305 from KennyStryker:main cc7ebe1 PiperOrigin-RevId: 776289436
Merged in fee1e2d |
✨ Add
sync
argument todeploy_index
method in Matching Engine IndexEndpointFixes #5304 🦕
Description
This PR introduces a new
sync
parameter to thedeploy_index
method, enabling both synchronous and asynchronous deployments. By default, the method remains synchronous to preserve backward compatibility.Changes
sync: bool = True
argument to thedeploy_index
method.sync
isTrue
, the method waits for the long-running deployment operation (LRO) to complete before returning.sync
isFalse
, the method returns immediately, and the LRO completes in the background.Usage Example
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Note: do not merge your PR from GitHub. Adding the "ready to pull" label is the final step in the review process.
After approvals, the changes in your PR will be committed to the
main
branch and this PR will be closed.