[v0/v1 migration] Migrate get_variable_ancestors from v1 to v2 API#6144
[v0/v1 migration] Migrate get_variable_ancestors from v1 to v2 API#6144SandeepTuniki merged 17 commits intomasterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the backend service to consume the v2/node API for retrieving statistical variable ancestors. By replacing the legacy v1 endpoint with a custom traversal implementation, the system gains more control over the hierarchy resolution process, including specific tie-breaking rules and cycle detection, while maintaining performance through added caching. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the get_variable_ancestors function to manually traverse the variable hierarchy using v2node calls instead of a single API endpoint. The new implementation includes caching, cycle detection, and specific tie-breaking logic for custom prefixes. Feedback suggests improving code maintainability by reusing the _get_all_values helper and simplifying the tie-breaking logic using a generator expression.
…onOf arcs concurrently
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request replaces the direct call to the /v1/variable/ancestors endpoint with a manual traversal of the variable hierarchy using memberOf and specializationOf relationships, incorporating caching and cycle detection. The reviewer noted that the use of asyncio.run within a loop is inefficient and recommended simplifying the logic by removing the asynchronous overhead and utilizing existing helper functions for response parsing.
juliawu
left a comment
There was a problem hiding this comment.
Nice! Just left a suggestion to consider.
Witholding approval temporarily until we get feature flags set up, just so this PR does not get merged in the meantime.
…to migrate-v1-variable-ancestors-to-v2-node
…to migrate-v1-variable-ancestors-to-v2-node
nick-nlb
left a comment
There was a problem hiding this comment.
I haven't yet done a full run through but a quick initial review before we meet!
nick-nlb
left a comment
There was a problem hiding this comment.
Added in one comment (not a major one) - the most important one is from the previous review batch (about the missing endpoint for v1 in discovery). Some of the test failures may relate to that, as environments that don't use v2 wouldn't be able to use the old fallback endpoint.
nick-nlb
left a comment
There was a problem hiding this comment.
Thank you Sandeep and Julia! LGTM
@SandeepTuniki Just a note that this PR was requested as a merge from a branch directly in upstream (the primary DC repo), instead of from your own fork.
|
Thanks, Julia & NIck. Merging this PR. |
@nick-nlb Yes, I usually just create a branch in the repo instead of a fork, to keep the development flow straightforward. Is there a different convention we follow for this repo? |
This PR migrates the website code from consuming
/v1/variable/ancestors/endpoint tov2/nodeapi (b/459854663).Instead of a single call to the
/v1/variable/ancestors/endpoint, we now make multiple calls tov2/nodeendpoint to fetch one ancestor at a time and return all of them in a single list to the frontend.I evaluated the possibility of leveraging the v2/node endpoint's
specializationOf+feature that recursively traverses the entire hierarchy at once, but found that the endpoint returns the ancestors in an alphabetically sorted order, so the direct parent-child edge relationships within that recursive chain are lost in the response. So instead, I went with repeated calls approach and memoized it to reduce latency of repeated lookups.