feat: Add support for reading selective GAPIC generation methods from service YAML #2272

gkevinzheng · 2024-12-04T16:07:38Z

This PR implements Selective GAPIC generation for Python. For more details, see:

Overall design doc: go/selective-gapic-generation
Implementation doc for Python: go/selective-gapic-gen-python

snippet-bot · 2024-12-04T16:22:16Z

Here is the summary of changes.

You are about to add 10 region tags.

tests/integration/goldens/redis_selective/samples/generated_samples/redis_v1_generated_cloud_redis_create_instance_async.py:26, tag redis_v1_generated_CloudRedis_CreateInstance_async
tests/integration/goldens/redis_selective/samples/generated_samples/redis_v1_generated_cloud_redis_create_instance_sync.py:26, tag redis_v1_generated_CloudRedis_CreateInstance_sync
tests/integration/goldens/redis_selective/samples/generated_samples/redis_v1_generated_cloud_redis_delete_instance_async.py:26, tag redis_v1_generated_CloudRedis_DeleteInstance_async
tests/integration/goldens/redis_selective/samples/generated_samples/redis_v1_generated_cloud_redis_delete_instance_sync.py:26, tag redis_v1_generated_CloudRedis_DeleteInstance_sync
tests/integration/goldens/redis_selective/samples/generated_samples/redis_v1_generated_cloud_redis_get_instance_async.py:26, tag redis_v1_generated_CloudRedis_GetInstance_async
tests/integration/goldens/redis_selective/samples/generated_samples/redis_v1_generated_cloud_redis_get_instance_sync.py:26, tag redis_v1_generated_CloudRedis_GetInstance_sync
tests/integration/goldens/redis_selective/samples/generated_samples/redis_v1_generated_cloud_redis_list_instances_async.py:26, tag redis_v1_generated_CloudRedis_ListInstances_async
tests/integration/goldens/redis_selective/samples/generated_samples/redis_v1_generated_cloud_redis_list_instances_sync.py:26, tag redis_v1_generated_CloudRedis_ListInstances_sync
tests/integration/goldens/redis_selective/samples/generated_samples/redis_v1_generated_cloud_redis_update_instance_async.py:26, tag redis_v1_generated_CloudRedis_UpdateInstance_async
tests/integration/goldens/redis_selective/samples/generated_samples/redis_v1_generated_cloud_redis_update_instance_sync.py:26, tag redis_v1_generated_CloudRedis_UpdateInstance_sync

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

parthea

LGTM but holding off on formal approval until tomorrow after the next release is shipped

parthea · 2024-12-18T18:22:00Z

Requesting review from @vchudnov-g as well

vchudnov-g

I think this is looking good, but I have some questions/suggestions. PTAL.

vchudnov-g · 2024-12-19T22:04:48Z

gapic/schema/api.py

            return self.disambiguate(f'_{string}')
        return string

+    def build_address_allowlist_for_selective_gapic(self, *,


In our code base (including comments), I would use a clearer name that "selective GAPIC"; I find it a bit of a misnomer. We're not selecting the GAPIC; we're selecting the RPCs to create a custom (non-standard) GAPIC. So I would have us choose one of selective_generation or custom_gapic and use that as the affix consistently everywhere, instead of selective_gapic.

vchudnov-g · 2024-12-19T22:11:30Z

gapic/schema/api.py

            return self.disambiguate(f'_{string}')
        return string

+    def build_address_allowlist_for_selective_gapic(self, *,


(I've not seen the word address used in this context and I wish we weren't using it, but I see it's prior art already in use. Oh, well)

vchudnov-g · 2024-12-19T22:18:14Z

gapic/schema/api.py

+        }
+
+        # For messages and enums we should only be pruning them from all_messages if they
+        # are proto plus types. This should apply to the Protos we are pruning from, but might


"but might not in the future."

WDYM? This seems cryptic.

Are we trying to move away from proto plus types? I'll remove the "in the future part" regardless

gapic/schema/api.py

gapic/schema/wrappers.py

Co-authored-by: Victor Chudnovsky <vchudnov@google.com>

vchudnov-g · 2025-01-23T00:16:54Z

Meta-comment: please do not merge main while a PR is being reviewed; it makes it much harder to see the incremental changes relevant only to the PR. I suggest merging main before sending out for the initial review, and after the PR is approved when you're ready to commit the PR to main (and if the merge changes are non-trivial, a second round of review could be reasonable, but that seems to be rare).

gapic/schema/api.py

gapic/schema/wrappers.py

tests/integration/redis_v1.yaml

gapic/schema/api.py

vchudnov-g · 2025-01-24T22:44:56Z

tests/unit/schema/test_api.py

+
+def test_python_settings_selective_gapic_version_mismatch_method_raises_error():
+    """
+    Test that `ClientLibrarySettingsError` is raised when there are nonexistent methods in


To highlight the difference with the previous test:

Suggested change

Test that `ClientLibrarySettingsError` is raised when there are nonexistent methods in

Test that `ClientLibrarySettingsError` is raised when a method listed for selective generation exists only in a different version of the library

vchudnov-g · 2025-01-25T00:00:30Z

tests/unit/schema/test_api.py

+    assert len(api_schema.all_protos) == 3
+    assert len(api_schema.protos) == 2


To make this more informative to someone figuring out this part of our architecture (including me, as I double-check my understanding I gleaned from looking at your PR), could you add some comments? In particular, is the following correct?

Suggested change

assert len(api_schema.all_protos) == 3

assert len(api_schema.protos) == 2

assert len(api_schema.all_protos) == 3 # foo.proto, common.proto, dep.proto

assert len(api_schema.protos) == 2 # foo.proto, common.proto

tests/unit/schema/test_api.py

tests/integration/redis_v1.yaml

vchudnov-g

This looks great! Please address open comments from previous reviews before you merge. Thank you so much for doing this!

vchudnov-g · 2025-01-27T17:48:07Z

tests/unit/schema/test_api.py

+    assert 'bar.proto' not in api_schema.protos
+    assert 'bar_common.proto' not in api_schema.protos
+
+    # Check that the sub-packages have been completely pruned are excluded from generation,


Suggested change

# Check that the sub-packages have been completely pruned are excluded from generation,

# Check that the sub-packages that have been completely pruned are excluded from generation,

(I accidentally omitted this word in my suggestion in a previous round of review)

tests/integration/goldens/redis_selective/.flake8

gkevinzheng requested a review from parthea December 4, 2024 16:07

gkevinzheng requested a review from a team as a code owner December 4, 2024 16:07

blunderbuss-gcf bot assigned vchudnov-g Dec 4, 2024

product-auto-label bot added the size: xl Pull request size is extra large. label Dec 4, 2024

gkevinzheng force-pushed the selective-gapic-p1 branch from 8f9db43 to 2274254 Compare December 11, 2024 20:04

gkevinzheng added 5 commits December 11, 2024 20:15

Added docstrings and fixed mypy

7c6fbae

Another attempt at fixing mypy

951af98

autopep8

1d64660

fix mypy + autopep8

4253c54

Fixed golden tests for new debug logging changes

55d0cca

gkevinzheng force-pushed the selective-gapic-p1 branch from 2274254 to 55d0cca Compare December 11, 2024 20:17

gkevinzheng and others added 2 commits December 11, 2024 20:30

Fixed import statements and golden test post-merge

7acbbf2

Merge branch 'main' into selective-gapic-p1

49b6177

parthea reviewed Dec 11, 2024

View reviewed changes

Merge branch 'main' into selective-gapic-p1

0021c48

parthea approved these changes Dec 18, 2024

View reviewed changes

parthea requested a review from vchudnov-g December 18, 2024 18:22

parthea changed the title ~~feat: Selective GAPIC phase 1~~ feat: Add support for reading selective GAPIC generation methods from service YAML Dec 18, 2024

parthea mentioned this pull request Dec 18, 2024

feat: Add support for reading selective GAPIC generation methods from service YAML #2135

Closed

vchudnov-g reviewed Dec 19, 2024

View reviewed changes

gkevinzheng and others added 7 commits December 20, 2024 09:46

Spelling

bb0d3aa

Co-authored-by: Victor Chudnovsky <vchudnov@google.com>

Update gapic/schema/wrappers.py

ee624af

Co-authored-by: Victor Chudnovsky <vchudnov@google.com>

Update gapic/schema/api.py

a9f4925

Co-authored-by: Victor Chudnovsky <vchudnov@google.com>

Merge branch 'main' into selective-gapic-p1

f975b1a

Address review feedback from vchudnov-g

0a8c041

Merge branch 'main' into selective-gapic-p1

6df8bdc

Merge branch 'main' into selective-gapic-p1

b5d6e15

vchudnov-g reviewed Jan 23, 2025

View reviewed changes

gkevinzheng added 3 commits January 24, 2025 18:10

Addressed more feedback

15cb53c

Updated redis_selective integration tests

679b62a

linting

4d91fb8

vchudnov-g reviewed Jan 24, 2025

View reviewed changes

gapic/schema/api.py Outdated Show resolved Hide resolved

gapic/schema/api.py Outdated Show resolved Hide resolved

vchudnov-g reviewed Jan 25, 2025

View reviewed changes

Addressed more feedback from vchudnov-g

419ecbb

vchudnov-g approved these changes Jan 27, 2025

View reviewed changes

gkevinzheng added 2 commits January 27, 2025 22:49

Final bit of feedback

7efbcd6

linting

3025652

gkevinzheng added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 27, 2025

gkevinzheng and others added 2 commits January 28, 2025 09:49

Merge branch 'main' into selective-gapic-p1

3d9d63a

Fixed integration test after merging with main

35afc8f

gkevinzheng merged commit 3a1a91c into main Jan 28, 2025
125 checks passed

gkevinzheng deleted the selective-gapic-p1 branch January 28, 2025 15:04

release-please bot mentioned this pull request Jan 24, 2025

chore(main): release 1.22.0 #2302

Merged

	Test that `ClientLibrarySettingsError` is raised when there are nonexistent methods in
	Test that `ClientLibrarySettingsError` is raised when a method listed for selective generation exists only in a different version of the library

		assert len(api_schema.all_protos) == 3
		assert len(api_schema.protos) == 2

	# Check that the sub-packages have been completely pruned are excluded from generation,
	# Check that the sub-packages that have been completely pruned are excluded from generation,

feat: Add support for reading selective GAPIC generation methods from service YAML #2272

feat: Add support for reading selective GAPIC generation methods from service YAML #2272

Uh oh!

Conversation

gkevinzheng commented Dec 4, 2024

Uh oh!

snippet-bot bot commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parthea left a comment

Choose a reason for hiding this comment

Uh oh!

parthea commented Dec 18, 2024

Uh oh!

vchudnov-g left a comment

Choose a reason for hiding this comment

Uh oh!

vchudnov-g Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

vchudnov-g Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

vchudnov-g Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

gkevinzheng Dec 20, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vchudnov-g commented Jan 23, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vchudnov-g Jan 24, 2025

Choose a reason for hiding this comment

Uh oh!

vchudnov-g Jan 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vchudnov-g left a comment

Choose a reason for hiding this comment

Uh oh!

vchudnov-g Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

snippet-bot bot commented Dec 4, 2024 •

edited

Loading

vchudnov-g Jan 25, 2025 •

edited

Loading