Visitar URL original
chore: add cross_sync annotations by daniel-sanche · Pull Request #1000 · googleapis/python-bigtable · GitHub
Skip to content
This repository was archived by the owner on Aug 30, 2026. It is now read-only.

chore: add cross_sync annotations - #1000

Merged
daniel-sanche merged 341 commits into
mainfrom
cross_sync2_pr2_annotations
Nov 20, 2024
Merged

chore: add cross_sync annotations#1000
daniel-sanche merged 341 commits into
mainfrom
cross_sync2_pr2_annotations

Conversation

@daniel-sanche

@daniel-sanche daniel-sanche commented Jul 18, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@daniel-sanche
removed old sync tests
9cf3923
@daniel-sanche
got unit tests passing
c59eec2
@daniel-sanche
Merge branch 'main' into sync_generator_cross_sync
fac2583
@daniel-sanche
updated sync files
8ef9047
@daniel-sanche
got sharding working with cross_sync
0705ee9
@daniel-sanche
fixed mypy errors
426057f
@daniel-sanche
ran blacken
0c79e39
@daniel-sanche
generate sync unit tests
4aa53eb
@daniel-sanche
improved import generation
6ae2428
@daniel-sanche
fixed cross sync import conditional
fdce0bc
@daniel-sanche
fixed import
adc8bb7
@daniel-sanche
got rpc tests passing
31fb77a
@daniel-sanche
removed custom is_async
fc44b30
@daniel-sanche
support dropping methods
255e124
@daniel-sanche
got tests passing
87aecb3
@daniel-sanche
got test_client sync tests passing
a1426a5
@daniel-sanche
dded simplified transformer to crosssync
ba351e3
@daniel-sanche
strip out basic sync code
d6fac8e
@daniel-sanche
include headers
d25a517
@daniel-sanche
add file imports
5dd32fd
@daniel-sanche
refactoring
cd40ba9
@daniel-sanche
fixed docstrings
fd639e4
@daniel-sanche
made changes to docstring format in generated files
c1053e9
@daniel-sanche
simplified generator visit
c925760
@daniel-sanche
replace string types
9f9ec0f
@daniel-sanche
add mypyy disabling
e5168a1
@daniel-sanche
regenerated imports
9bd13b3
@daniel-sanche
got rename_sync decorator working
6895968
@daniel-sanche
made convert decorator
09c090d
@daniel-sanche
added CrossSync.Awaitable
ab28899
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 November 12, 2024 00:25
@daniel-sanche daniel-sanche assigned mutianf and unassigned kongweihan Nov 12, 2024
mutianf
mutianf reviewed Nov 14, 2024
View reviewed changes

@mutianf mutianf left a comment

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.

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

Comment thread google/cloud/bigtable/data/_async/_mutate_rows.py
Comment thread google/cloud/bigtable/data/_async/client.py Outdated
),
"RAISE_NO_LOOP": (
"RuntimeError: if called outside of an async context (no running event loop)",
"",

@mutianf mutianf Oct 25, 2024

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.

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

Comment thread google/cloud/bigtable/data/__init__.py
Comment thread google/cloud/bigtable/data/_async/_read_rows.py
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

@mutianf mutianf Nov 14, 2024

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.

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

@daniel-sanche daniel-sanche Nov 14, 2024

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.

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

Comment thread google/cloud/bigtable/data/execute_query/_async/execute_query_iterator.py Outdated
Comment thread tests/system/data/test_system_async.py Outdated
Comment thread tests/unit/data/_async/test__mutate_rows.py Outdated
Comment thread tests/unit/data/_async/test_mutations_batcher.py Outdated
Comment thread tests/unit/data/_async/test_mutations_batcher.py
@daniel-sanche

daniel-sanche commented Nov 15, 2024

Copy link
Copy Markdown
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
@daniel-sanche
daniel-sanche deleted the cross_sync2_pr2_annotations branch November 20, 2024 00:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@mutianf mutianf mutianf approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mutianf mutianf

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@daniel-sanche @mutianf @kongweihan