Skip to content

feat: Added matching engine integration#995

Merged
ivanmkc merged 71 commits into
googleapis:mainfrom
ivanmkc:imkc--matching-engine
Mar 10, 2022
Merged

feat: Added matching engine integration#995
ivanmkc merged 71 commits into
googleapis:mainfrom
ivanmkc:imkc--matching-engine

Conversation

@ivanmkc
Copy link
Copy Markdown
Contributor

@ivanmkc ivanmkc commented Feb 2, 2022

Added matching engine integration.

Tasks

  • Add Index and IndexEndpoint classes with create, get, list, update, delete methods
  • Add IndexEndpoint.deploy_index() method that accepts Index object
  • IndexEndpoint.undeploy_index
  • IndexEndpoint.mutate_deployed_index
  • IndexEndpoint.metadata.config parameters
  • IndexEndpoint.undeploy_all
  • Prediction/matching methods.

Testing

  • Unit tests
  • Integration test

Potential convenience methods

  • MatchingEngineIndexEndpoint.delete(..., force: bool)

Note

MatchingEngineIndexEndpoint.match could not be unit tested due to https://stackoverflow.com/questions/47202355/pytest-cannot-mock-init-of-my-class.

It's been tested in the integration test.

Fixes b/201313324, #994

@ivanmkc ivanmkc requested review from morgandu and sasha-gitg and removed request for morgandu and sasha-gitg February 2, 2022 17:48
@ivanmkc ivanmkc force-pushed the imkc--matching-engine branch 4 times, most recently from 5b0b460 to b45842b Compare February 15, 2022 22:14
@ivanmkc ivanmkc changed the title [WIP] Added matching engine integration feat: Added matching engine integration Feb 15, 2022
@ivanmkc ivanmkc force-pushed the imkc--matching-engine branch 2 times, most recently from 2e2f144 to 6cfc93d Compare February 16, 2022 01:53
Copy link
Copy Markdown

@ReneeZhuGG ReneeZhuGG left a comment

Choose a reason for hiding this comment

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

LGTM, left a few minor comments.

Comment thread google/cloud/aiplatform/matching_engine/matching_engine_index_endpoint.py Outdated
Comment thread google/cloud/aiplatform/matching_engine/matching_engine_index_endpoint.py Outdated
Comment thread google/cloud/aiplatform/matching_engine/matching_engine_index_endpoint.py Outdated
Comment thread google/cloud/aiplatform/matching_engine/matching_engine_index_endpoint.py Outdated
Comment thread google/cloud/aiplatform/matching_engine/matching_engine_index_endpoint.py Outdated
@ivanmkc ivanmkc force-pushed the imkc--matching-engine branch 4 times, most recently from b069195 to f83d533 Compare February 19, 2022 01:33
Comment thread google/cloud/aiplatform/matching_engine/match_service_pb2_grpc.py
Comment thread google/cloud/aiplatform/matching_engine/match_service_pb2_grpc.py
Comment thread google/cloud/aiplatform/matching_engine/matching_engine_index.py Outdated
Comment thread google/cloud/aiplatform/utils/__init__.py Outdated
Comment thread google/cloud/aiplatform/__init__.py Outdated
Comment thread google/cloud/aiplatform/compat/services/__init__.py Outdated
Comment thread google/cloud/aiplatform/matching_engine/__init__.py Outdated
Comment thread google/cloud/aiplatform/utils/__init__.py
Comment thread google/cloud/aiplatform/matching_engine/matching_engine_index.py
Comment thread google/cloud/aiplatform/matching_engine/matching_engine_index.py
"index_endpoint", "undeployed", self
)

def undeploy_all(self, sync: bool = True) -> "MatchingEngineIndexEndpoint":
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

metadata?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Endpoint.undeploy_all doesn't have it so I followed that.

This loops through each. deployed_index and undeploys it.
Do you propose sending the same metadata for every deploy call?

@morgandu
Copy link
Copy Markdown
Contributor

Thanks for the great work @ivanmkc , left some comments, mostly minor issue, except one pattern on the resource operation assuming resource is successfully created. Since the resource creation is optional_sync supported, any immediate operation on a creating resource may lead to error i.e. self.resource_name is not available yet. One option is add self.wait() in all downstream operations, this will be probably better if the resource creation takes time. Another option is remove the optional_sync from resource creation.

@ivanmkc ivanmkc force-pushed the imkc--matching-engine branch from f117cb5 to e621402 Compare February 23, 2022 23:37
Comment thread tests/system/aiplatform/test_matching_engine_index.py Outdated
Comment thread tests/unit/aiplatform/test_matching_engine_index.py Outdated
@ivanmkc ivanmkc force-pushed the imkc--matching-engine branch 2 times, most recently from 53e32da to d6d8af3 Compare February 25, 2022 20:26
@ivanmkc ivanmkc force-pushed the imkc--matching-engine branch 3 times, most recently from 63f84be to c4fb6a2 Compare March 4, 2022 14:25
@ivanmkc ivanmkc force-pushed the imkc--matching-engine branch from 01cfdfa to 5b34124 Compare March 10, 2022 15:12
@ivanmkc ivanmkc merged commit bbe105d into googleapis:main Mar 10, 2022
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.

3 participants