Add alloydb vector search codeowners#14441
Conversation
Added @GoogleCloudPlatform/alloydb-vector-search-docs as a co-owner of the /alloydb/ directory.
There was a problem hiding this comment.
Code Review
This pull request adds the alloydb-vector-search-docs team to the CODEOWNERS file for the alloydb directory and introduces a new test, test_alloydb_vector_search_benchmark, in alloydb/notebooks/e2e_test.py to run a benchmark notebook with reduced parameters. Feedback suggests making the new test synchronous as it lacks asynchronous operations, and using repr(password) instead of string interpolation to safely handle special characters in passwords.
| @pytest.mark.asyncio | ||
| async def test_alloydb_vector_search_benchmark( | ||
| project_id: str, | ||
| cluster_name: str, | ||
| instance_name: str, | ||
| region: str, | ||
| database_name: str, | ||
| password: str, | ||
| ) -> None: | ||
| # Run the benchmark notebook with significantly reduced parameters | ||
| # to allow CI to complete quickly without OOMs or timeouts | ||
| conftest.run_notebook( | ||
| "alloydb_vector_search_benchmark.ipynb", | ||
| variables={ | ||
| "project_id": project_id, | ||
| "region": region, | ||
| "cluster_id": cluster_name, | ||
| "instance_id": instance_name, | ||
| "db_name": database_name, | ||
| "db_user": "postgres", | ||
| "test_queries": 10, | ||
| }, | ||
| preprocess=preprocess, | ||
| skip_shell_commands=True, | ||
| replace={ | ||
| ( | ||
| "db_pass = getpass.getpass(" | ||
| '"🔑 Enter Database Password for AlloyDB: ")' | ||
| ): f"db_pass = '{password}'", | ||
| "train_data = f['train'][:]": "train_data = f['train'][:1000]", | ||
| "test_data = f['test'][:]": "test_data = f['test'][:100]", | ||
| "neighbors_data = f['neighbors'][:]": ( | ||
| "neighbors_data = f['neighbors'][:100]" | ||
| ), | ||
| "ef_values = [40, 100, 200, 400, 800]": "ef_values = [40]", | ||
| }, | ||
| until_end=True, | ||
| ) |
There was a problem hiding this comment.
The test function test_alloydb_vector_search_benchmark is defined as an asynchronous function (async def) and decorated with @pytest.mark.asyncio, but it does not contain any await expressions or asynchronous operations. Making it synchronous simplifies the test and avoids unnecessary async overhead.
Additionally, using f"db_pass = '{password}'" can cause syntax errors or test failures if the password contains single quotes or backslashes. Using repr(password) is safer and more robust as it automatically handles escaping and quotes.
def test_alloydb_vector_search_benchmark(
project_id: str,
cluster_name: str,
instance_name: str,
region: str,
database_name: str,
password: str,
) -> None:
# Run the benchmark notebook with significantly reduced parameters
# to allow CI to complete quickly without OOMs or timeouts
conftest.run_notebook(
"alloydb_vector_search_benchmark.ipynb",
variables={
"project_id": project_id,
"region": region,
"cluster_id": cluster_name,
"instance_id": instance_name,
"db_name": database_name,
"db_user": "postgres",
"test_queries": 10,
},
preprocess=preprocess,
skip_shell_commands=True,
replace={
(
"db_pass = getpass.getpass("
'"🔑 Enter Database Password for AlloyDB: ")'
): f"db_pass = {repr(password)}",
"train_data = f['train'][:]": "train_data = f['train'][:1000]",
"test_data = f['test'][:]": "test_data = f['test'][:100]",
"neighbors_data = f['neighbors'][:]": (
"neighbors_data = f['neighbors'][:100]"
),
"ef_values = [40, 100, 200, 400, 800]": "ef_values = [40]",
},
until_end=True,
)
Description
Fixes #
Checklist
Testing
Compliance & Style
Post-Approval Actions