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

chore: add cross_sync annotations #1000

Merged
merged 341 commits into from
Nov 20, 2024
Merged

Conversation

daniel-sanche
Copy link
Contributor

@daniel-sanche daniel-sanche commented Jul 18, 2024

PR 2/x for adding the sync surface for the new data client

This PR restructures the existing async library to better support the sync side. Specifically:

  • all async-specific calls and types are replaced with corresponding calls to the new CrossSync class, which simply contains a set of aliases pointing back at the corresponding async functionality
  • all async keywords are annotated with CrossSync.rm_aio, which tells the ast transformers where to strip out asyncio keywords

Currently, we need to run the conversion process manually (using python .cross_sync/generate.py, and the sync files are not included in the codebase. The next PR will add the sync artifacts, and add the generation pipeline into the CI system

Base automatically changed from cross_sync2_pr1_architecture to main November 8, 2024 18:37
@daniel-sanche daniel-sanche changed the title [DRAFT] chore: add cross_sync annotations chore: add cross_sync annotations Nov 12, 2024
@daniel-sanche daniel-sanche marked this pull request as ready for review November 12, 2024 00:25
@daniel-sanche daniel-sanche requested review from a team as code owners November 12, 2024 00:25
@daniel-sanche daniel-sanche assigned mutianf and unassigned kongweihan Nov 12, 2024
Copy link
Contributor

@mutianf mutianf left a comment

Choose a reason for hiding this comment

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

Is there a test to validate that the sync surface is re-generated when there's change to async code?

),
"RAISE_NO_LOOP": (
"RuntimeError: if called outside of an async context (no running event loop)",
"",
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, let's change this to None to be consistent with other methods?

google/cloud/bigtable/data/__init__.py Show resolved Hide resolved
if CrossSync.is_async:
from google.cloud.bigtable.data._async.client import TableAsync as TableType
else:
from google.cloud.bigtable.data._sync_autogen.client import Table as TableType # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

dumb question.. isn't _sync_autogen.client also generated from cross sync? how can we import a not yet generated class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good point, this should really just be in the next PR, since the file doesn't exist yet

For the "how" part: this import statement isn't actually executed, since it's behind an if False branch. This import is just for the generated file

tests/system/data/test_system_async.py Outdated Show resolved Hide resolved
tests/unit/data/_async/test__mutate_rows.py Outdated Show resolved Hide resolved
tests/unit/data/_async/test_mutations_batcher.py Outdated Show resolved Hide resolved
@daniel-sanche
Copy link
Contributor Author

Is there a test to validate that the sync surface is re-generated when there's change to async code?

Yes, but that's in the next PR, since we don't have the generated files here yet

@daniel-sanche daniel-sanche added the cla: yes This human has signed the Contributor License Agreement. label Nov 20, 2024
@daniel-sanche daniel-sanche merged commit 7ea3c23 into main Nov 20, 2024
30 of 33 checks passed
@daniel-sanche daniel-sanche deleted the cross_sync2_pr2_annotations branch November 20, 2024 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the googleapis/python-bigtable API. cla: yes This human has signed the Contributor License Agreement. size: xl Pull request size is extra large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants