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 delete 12 region tags.

tests/integration/goldens/redis/samples/generated_samples/redis_v1_generated_cloud_redis_export_instance_async.py:26, tag redis_v1_generated_CloudRedis_ExportInstance_async
tests/integration/goldens/redis/samples/generated_samples/redis_v1_generated_cloud_redis_export_instance_sync.py:26, tag redis_v1_generated_CloudRedis_ExportInstance_sync
tests/integration/goldens/redis/samples/generated_samples/redis_v1_generated_cloud_redis_failover_instance_async.py:26, tag redis_v1_generated_CloudRedis_FailoverInstance_async
tests/integration/goldens/redis/samples/generated_samples/redis_v1_generated_cloud_redis_failover_instance_sync.py:26, tag redis_v1_generated_CloudRedis_FailoverInstance_sync
tests/integration/goldens/redis/samples/generated_samples/redis_v1_generated_cloud_redis_get_instance_auth_string_async.py:26, tag redis_v1_generated_CloudRedis_GetInstanceAuthString_async
tests/integration/goldens/redis/samples/generated_samples/redis_v1_generated_cloud_redis_get_instance_auth_string_sync.py:26, tag redis_v1_generated_CloudRedis_GetInstanceAuthString_sync
tests/integration/goldens/redis/samples/generated_samples/redis_v1_generated_cloud_redis_import_instance_async.py:26, tag redis_v1_generated_CloudRedis_ImportInstance_async
tests/integration/goldens/redis/samples/generated_samples/redis_v1_generated_cloud_redis_import_instance_sync.py:26, tag redis_v1_generated_CloudRedis_ImportInstance_sync
tests/integration/goldens/redis/samples/generated_samples/redis_v1_generated_cloud_redis_reschedule_maintenance_async.py:26, tag redis_v1_generated_CloudRedis_RescheduleMaintenance_async
tests/integration/goldens/redis/samples/generated_samples/redis_v1_generated_cloud_redis_reschedule_maintenance_sync.py:26, tag redis_v1_generated_CloudRedis_RescheduleMaintenance_sync
tests/integration/goldens/redis/samples/generated_samples/redis_v1_generated_cloud_redis_upgrade_instance_async.py:26, tag redis_v1_generated_CloudRedis_UpgradeInstance_async
tests/integration/goldens/redis/samples/generated_samples/redis_v1_generated_cloud_redis_upgrade_instance_sync.py:26, tag redis_v1_generated_CloudRedis_UpgradeInstance_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

@@ -237,6 +237,69 @@ def disambiguate(self, string: str) -> str:
            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

@@ -237,6 +237,69 @@ def disambiguate(self, string: str) -> str:
            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

vchudnov-g · 2024-12-19T22:46:00Z

gapic/schema/api.py

+
+                # We only prune services/messages/enums from protos that are not depepdencies.
+                for name, proto in api.all_protos.items():
+                    if name not in api.protos:


What does it mean for a proto to be in api.all_protos but not in api.protos? (I'm not familiar with this part of the codebase)

Definition here:

gapic-generator-python/gapic/schema/api.py

Lines 406 to 417 in e050f4e

def protos(self) -> Mapping[str, Proto]:

"""Return a map of all protos specific to this API.

This property excludes imported protos that are dependencies

of this API but not being directly generated.

"""

view = self.subpackage_view

return collections.OrderedDict([

(k, v) for k, v in self.all_protos.items()

if v.file_to_generate and

v.meta.address.subpackage[:len(view)] == view

])

Here I'm trying to prune only the proto objects corresponding to the files in this API package.

gapic/schema/wrappers.py

vchudnov-g · 2024-12-19T23:01:23Z

gapic/schema/wrappers.py

+        if self.extended_lro:
+            # We need to add the service/method pointed to by self.operation_service to
+            # the allowlist, as it might not have been specified by selective_gapic_generation.
+            # We assume that the operation service lives in the same proto as this one.


Is this a good assumption? (I honestly don't know.) Will it work with operation mixins?

I have to make this assumption, otherwise the lookups won't work. The precedent for this is here where the Proto object is built:

gapic-generator-python/gapic/schema/api.py

Lines 1238 to 1250 in e050f4e

def _maybe_get_extended_lro(

self,

service_address: metadata.Address,

meth_pb: descriptor_pb2.MethodDescriptorProto,

) -> Optional[wrappers.ExtendedOperationInfo]:

op_service_name = meth_pb.options.Extensions[ex_ops_pb2.operation_service]

if not op_service_name:

return None

# Manual lookups because services and methods haven't been loaded.

# Note: this assumes that the operation service lives in the same proto file.

# This is a reasonable assumption as of March '22, but it may (unlikely)

# change in the future.

vchudnov-g · 2024-12-19T23:16:12Z

gapic/schema/wrappers.py

+        for method in self.methods.values():
+            if method.ident.proto in method_names:
+                # Include this service if there are any types/methods in selective gapic for this service.
+                address_allowlist.add(self.meta.address)


Isn't this allow-listing this service it it has any methods at all, not just allowlisted ones?

Should the logic be something like this pseudocode?

for method in self.methods.values(): if method in allowlist: allowlist.add(self) allowlist.add(method) method.build_allowlist()

method_names is the allowlist. I'll make that clearer by renaming it to method_allowlist or method_names_allowlist.

vchudnov-g · 2024-12-19T23:20:17Z

gapic/schema/wrappers.py

Looking at this file, it seems to me that the build_address_allowlist name is descriptive for the method you're adding to the Service class, but the other classes should have their new methods names something like add_to_address_allowlist, to emphasize that they themselves are not checking for the RPC allowlist in the YAML, but are being included transitively. WDYT?

Sounds good to me.

Co-authored-by: Victor Chudnovsky <[email protected]>

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 6 commits December 20, 2024 09:46

Spelling

bb0d3aa

Co-authored-by: Victor Chudnovsky <[email protected]>

Update gapic/schema/wrappers.py

ee624af

Co-authored-by: Victor Chudnovsky <[email protected]>

Update gapic/schema/api.py

a9f4925

Co-authored-by: Victor Chudnovsky <[email protected]>

Merge branch 'main' into selective-gapic-p1

f975b1a

Address review feedback from vchudnov-g

0a8c041

Merge branch 'main' into selective-gapic-p1

6df8bdc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

gkevinzheng commented Dec 4, 2024

snippet-bot bot commented Dec 4, 2024 •

edited

Loading

parthea left a comment

parthea commented Dec 18, 2024

vchudnov-g left a comment

vchudnov-g Dec 19, 2024

vchudnov-g Dec 19, 2024

vchudnov-g Dec 19, 2024

gkevinzheng Dec 20, 2024

vchudnov-g Dec 19, 2024

gkevinzheng Dec 20, 2024

vchudnov-g Dec 19, 2024

gkevinzheng Dec 20, 2024 •

edited

Loading

vchudnov-g Dec 19, 2024

gkevinzheng Dec 20, 2024

vchudnov-g Dec 19, 2024

gkevinzheng Dec 20, 2024

	def protos(self) -> Mapping[str, Proto]:
	"""Return a map of all protos specific to this API.

	This property excludes imported protos that are dependencies
	of this API but not being directly generated.
	"""
	view = self.subpackage_view
	return collections.OrderedDict([
	(k, v) for k, v in self.all_protos.items()
	if v.file_to_generate and
	v.meta.address.subpackage[:len(view)] == view
	])

	def _maybe_get_extended_lro(
	self,
	service_address: metadata.Address,
	meth_pb: descriptor_pb2.MethodDescriptorProto,
	) -> Optional[wrappers.ExtendedOperationInfo]:
	op_service_name = meth_pb.options.Extensions[ex_ops_pb2.operation_service]
	if not op_service_name:
	return None

	# Manual lookups because services and methods haven't been loaded.
	# Note: this assumes that the operation service lives in the same proto file.
	# This is a reasonable assumption as of March '22, but it may (unlikely)
	# change in the future.

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

Are you sure you want to change the base?

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

Conversation

gkevinzheng commented Dec 4, 2024

snippet-bot bot commented Dec 4, 2024 • edited Loading

parthea left a comment

Choose a reason for hiding this comment

parthea commented Dec 18, 2024

vchudnov-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkevinzheng Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snippet-bot bot commented Dec 4, 2024 •

edited

Loading

gkevinzheng Dec 20, 2024 •

edited

Loading