Skip to content

Clone of #5959#6047

Draft
SandeepTuniki wants to merge 5 commits intomasterfrom
migrate-place-info-v2
Draft

Clone of #5959#6047
SandeepTuniki wants to merge 5 commits intomasterfrom
migrate-place-info-v2

Conversation

@SandeepTuniki
Copy link

Don't review.

I'm recreating the PR #5959 since that one got messy.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello, 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 re-implements key Data Commons service functions, get_place_info and get_series_dates, to migrate them from the V1 to the V2 API. This migration aims to improve data retrieval mechanisms, particularly for hierarchical place information and observation series. The changes include comprehensive unit tests and a live verification script to ensure the robustness and correctness of the new V2 API integrations. This PR is a re-creation of a previous, "messy" pull request, and is not yet ready for review.

Highlights

  • V2 API Migration for get_place_info: The get_place_info function was completely refactored to utilize the V2 Data Commons API, implementing a Breadth-First Search (BFS) for ancestor retrieval and sorting parents by a new PLACE_TYPE_RANK system.
  • V2 API Migration for get_series_dates: The get_series_dates function was re-implemented to leverage the V2 Data Commons API, including filtering children by type and aggregating observation dates.
  • New Unit Tests: New unit tests were introduced for get_place_info and get_series_dates to validate their V2 API implementations, covering edge cases like recursion depth and cyclic dependencies.
  • Live Verification Script: A new live verification script was added to test the migrated functions in a Flask application context, ensuring their correct operation against the live Data Commons API.
Changelog
  • server/services/datacommons.py
    • Removed the old get_place_info function.
    • Added a PLACE_TYPE_RANK dictionary for sorting place types.
    • Implemented a new get_place_info function using V2 v2node API, including BFS for ancestor discovery, handling recursion limits, and sorting parents.
    • Removed the old get_series_dates function.
    • Implemented a new get_series_dates function using V2 v2node and v2observation APIs, including child filtering and observation aggregation.
  • server/tests/migration_verification_test.py
    • Added a new test file.
    • Included test_get_place_info_v2 to verify the new get_place_info implementation with mocked V2 API responses.
    • Included test_get_series_dates_v2 to verify the new get_series_dates implementation with mocked V2 API responses.
    • Added test_get_place_info_edge_cases to test recursion limits and cycles in get_place_info.
    • Added test_get_series_dates_error to test error handling in get_series_dates.
  • server/tests/verify_live_migration.py
    • Added a new script for live testing.
    • Implemented verify_get_place_info to test the get_place_info function against the live Data Commons API.
    • Implemented verify_get_series_dates to test the get_series_dates function against the live Data Commons API.
    • Included Flask app context setup for proper configuration loading during live verification.
Activity
  • The author, SandeepTuniki, has recreated this pull request as a cleaner version of a previous one (chore: Migrate Place Info and Series Dates to V2 API #5959) that became "messy".
  • The pull request is currently marked as "Don't review", indicating it is still in progress or not yet ready for formal review.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors get_place_info and get_series_dates to use V2 APIs instead of V1, which is a significant change involving the reimplementation of V1 endpoint logic by composing multiple V2 API calls. The changes also include new unit tests and a live verification script.

My review identified several issues that need attention:

  • A correctness bug in get_series_dates related to filtering child places by type.
  • An inefficiency in get_place_info that can be improved for better performance.
  • The unit tests for get_series_dates use incorrect mock data that doesn't match the real API response, which unfortunately hides the implementation bug.
  • The new live verification script contains a bug in its assertion logic, rendering it ineffective.

I have provided specific comments and code suggestions to address these points. While the overall direction of the changes is good, the implementation requires these fixes to ensure correctness and robustness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant