Repository navigation
chore(bigtable): run samples tests pre-submit - #18599
daniel-sanche wants to merge 12 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates relative imports across several Google Cloud Bigtable samples and adds new configuration files, including noxfile.py and requirements files, for the async data client samples. A critical issue was identified in packages/google-cloud-bigtable/samples/utils.py, where unresolved git merge conflict markers were left in the code, which will lead to a runtime SyntaxError.
a062620 to
36b085c
Compare
1cc22cd to
886ab68
Compare
To clarify , I clicked the |
| apache-beam==2.69.0; python_version == '3.9' | ||
| apache-beam==2.71.0; python_version >= '3.10' | ||
| google-cloud-bigtable==2.35.0 | ||
| google-cloud-bigtable |
There was a problem hiding this comment.
Please could you add a comment to clarify the reason we don't pin the google-cloud-bigtable library here, and elsewhere?
There was a problem hiding this comment.
We want these sample tests to run against the repo head, so we can catch if any of our changes break anything. Running against an old released version of the library isn't valuable
There was a problem hiding this comment.
In that case, perhaps we can remove the requirements files altogether, or only have the dependencies which are not in setup.py
We can then remove session.install("-e", ".", "--no-deps") from the noxfile.py file and have:
session.install("-e", ".")
session.install(
"google-cloud-testutils",
"mock",
"pytest",
"pytest-asyncio",
*req_args,
)
There was a problem hiding this comment.
I think the requrements are useful, even if just to help users understand/run the samples. They're referenced in the samples README, and could be seen as of part of the documentation
But if you prefer to get rid of them, I don't feel too strongly about it
| batcher = table.mutations_batcher(flush_count=2) | ||
| rows = table.read_rows() | ||
| for row in rows: | ||
| row = table.row(row.row_key) | ||
| row = table.direct_row(row.row_key) | ||
| row.delete_cell(column_family_id="cell_plan", column="data_plan_01gb") | ||
|
|
||
| batcher.mutate_rows(rows) | ||
| batcher.close() |
There was a problem hiding this comment.
Gemini suggested we should have this instead
change
batcher = table.mutations_batcher(flush_count=2)
rows = table.read_rows()
for row in rows:
row = table.direct_row(row.row_key)
row.delete_cell(column_family_id="cell_plan", column="data_plan_01gb")
batcher.mutate_rows(rows)
batcher.close()
to
with table.mutations_batcher(flush_count=2) as batcher:
for row in table.read_rows():
direct_row = table.direct_row(row.row_key)
direct_row.delete_cell(column_family_id="cell_plan", column="data_plan_01gb")
batcher.mutate(direct_row)
This test fails without the fix
def test_streaming_and_batching_actually_deletes(table_id):
"""Verifies that streaming_and_batching actually deletes cells from the table."""
from google.cloud import bigtable
from . import deletes_snippets
client = bigtable.Client(project=PROJECT, admin=True)
instance = client.instance(BIGTABLE_INSTANCE)
table = instance.table(table_id)
# 1. Seed a row in the table with cell_plan:data_plan_01gb
row_key = b"phone#4c410523#20190501"
row = table.direct_row(row_key)
row.set_cell("cell_plan", b"data_plan_01gb", b"true")
row.commit()
# 2. Verify row exists before calling the snippet
seeded_row = table.read_row(row_key)
assert seeded_row is not None, "Failed to seed row!"
assert b"data_plan_01gb" in seeded_row.cells["cell_plan"]
# 3. Run the snippet
deletes_snippets.streaming_and_batching(
PROJECT, BIGTABLE_INSTANCE, table_id
)
# 4. Check if the cell was deleted
updated_row = table.read_row(row_key)
# If the snippet worked, either the row is None (all cells deleted)
# or the column data_plan_01gb is absent from cell_plan:
has_cell = (
updated_row is not None
and b"data_plan_01gb" in updated_row.cells.get("cell_plan", {})
)
assert not has_cell, "Cell was NOT deleted because batcher received an exhausted generator!"
There was a problem hiding this comment.
Good catch, fixed!
The bigtable library lost the test configs for samples as part of the move to the monorepo, so they are left untested. This PR adds back the configs, and runs tests as part of the pre-submit check
Samples tests are now run from the central bigtable noxfile, instead of using individual noxfiles for each sample directory