[WIP] Experiment deterministic resource names#16569
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces changes to ensure deterministic output in the GAPIC generator and improves resource name collision resolution by prefixing foreign domains. Key updates include modifying resource_messages to return sorted sequences, updating Jinja2 templates to use disambiguated method names, and adding a new configuration for Dialogflow. Feedback includes a critical request to revert a hardcoded local path in the requirements file, a suggestion to use standard dictionaries for sorted items, and recommendations to remove redundant properties and move inline imports to the top level.
| @@ -1,5 +1,5 @@ | |||
| click | |||
| gapic-generator==1.30.13 # https://github.com/googleapis/gapic-generator-python/releases/tag/v1.30.13 | |||
| -e /usr/local/google/home/omairn/git/googleapis/google-cloud-python/packages/gapic-generator | |||
There was a problem hiding this comment.
The use of a hardcoded local absolute path (-e /usr/local/google/...) will break the build for other developers and in CI/CD environments. Please revert this to a versioned dependency or a relative path if necessary for the experiment.
gapic-generator==1.30.13 # https://github.com/googleapis/gapic-generator-python/releases/tag/v1.30.13
| return collections.OrderedDict( | ||
| itertools.chain( | ||
| file_resource_messages, | ||
| resource_messages, | ||
| sorted( | ||
| itertools.chain(file_resource_messages, resource_messages), | ||
| key=lambda item: item[0] | ||
| ) | ||
| ) |
There was a problem hiding this comment.
In modern Python (3.7+), the standard dict preserves insertion order. Following the general rules for this repository, you should use dict(sorted(...)) instead of collections.OrderedDict to ensure determinism more concisely.
| return collections.OrderedDict( | |
| itertools.chain( | |
| file_resource_messages, | |
| resource_messages, | |
| sorted( | |
| itertools.chain(file_resource_messages, resource_messages), | |
| key=lambda item: item[0] | |
| ) | |
| ) | |
| return dict( | |
| sorted( | |
| itertools.chain(file_resource_messages, resource_messages), | |
| key=lambda item: item[0] | |
| ) | |
| ) |
References
- To ensure dictionary keys remain sorted without manual effort, programmatically sort the dictionary before returning it (e.g., using dict(sorted(metadata.items()))) instead of relying on manual ordering in the code.
| @utils.cached_property | ||
| def deterministic_resource_messages(self) -> Sequence[MessageType]: | ||
| """Returns a sorted sequence of resource messages to guarantee deterministic output order.""" | ||
| return tuple( | ||
| sorted( | ||
| self.resource_messages, | ||
| key=lambda r: r.resource_type_full_path or r.name | ||
| ) | ||
| ) |
There was a problem hiding this comment.
The deterministic_resource_messages property appears to be redundant because the resource_messages property (defined later in this file) has also been updated to return a sorted sequence. Since the templates and tests now rely on resource_messages being deterministic, this extra property can be removed.
| import collections | ||
| from gapic import utils |
There was a problem hiding this comment.
Inline imports are generally discouraged unless they are necessary to avoid circular dependencies. collections is a standard library module that should be imported at the top level. Furthermore, utils is already available in the global scope of this module, making the local import redundant.
1 participant
Experiment