-
Notifications
You must be signed in to change notification settings - Fork 115
Automated Integration Test Goldens Update from CI #5871
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Automated Integration Test Goldens Update from CI #5871
Conversation
…232-a14a-8f455bfbcd1f)
Summary of ChangesHello @datacommons-robot-author, 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 automated integration test golden files to reflect recent changes in the system's query detection and statistical variable matching capabilities. The modifications primarily stem from enhancements in query parsing, including the selective integration of a Large Language Model for interpreting complex queries, and adjustments to the internal logging of query transformations. These updates ensure that the test suite accurately captures the current behavior of the data commons API. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. 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
|
There was a problem hiding this 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 updates integration test golden files. I've reviewed the changes and found a critical regression in one of the test cases. The detection for the query 'number of poor hispanic women with phd' is now completely broken and returns no results. Additionally, another test case now uses an empty string for sv_detection_query_index_types, which is suspicious and could indicate a bug or misconfiguration. These issues should be investigated before merging.
| "query_with_places_removed": "", | ||
| "sv_matching": { | ||
| "CosineScore": [ | ||
| 0.79858, | ||
| 0.78833, | ||
| 0.77635, | ||
| 0.77406, | ||
| 0.74103, | ||
| 0.73347, | ||
| 0.72283, | ||
| 0.72257, | ||
| 0.69955, | ||
| 0.6995, | ||
| 0.69762, | ||
| 0.69743, | ||
| 0.696, | ||
| 0.69534, | ||
| 0.69452, | ||
| 0.69396, | ||
| 0.69354, | ||
| 0.69084, | ||
| 0.6907, | ||
| 0.68967, | ||
| 0.68895, | ||
| 0.68862, | ||
| 0.68633, | ||
| 0.68592, | ||
| 0.68275, | ||
| 0.68221, | ||
| 0.68156, | ||
| 0.67987, | ||
| 0.67686, | ||
| 0.67631, | ||
| 0.67515, | ||
| 0.6745, | ||
| 0.67257, | ||
| 0.6718, | ||
| 0.66917, | ||
| 0.669, | ||
| 0.66822, | ||
| 0.66781, | ||
| 0.66507, | ||
| 0.66487 | ||
| ], | ||
| "MultiSV": { | ||
| "Candidates": [ | ||
| { | ||
| "AggCosineScore": 0.8315, | ||
| "DelimBased": false, | ||
| "Parts": [ | ||
| { | ||
| "CosineScore": [ | ||
| 0.83118, | ||
| 0.831, | ||
| 0.82552, | ||
| 0.81955, | ||
| 0.81456, | ||
| 0.80907, | ||
| 0.80864, | ||
| 0.80744, | ||
| 0.8003, | ||
| 0.79858, | ||
| 0.78782 | ||
| ], | ||
| "QueryPart": "number of poor hispanic", | ||
| "SV": [ | ||
| "Count_Person_BelowPovertyLevelInThePast12Months_HispanicOrLatino", | ||
| "Count_Person_HispanicOrLatino", | ||
| "Count_Person_Female_AbovePovertyLevelInThePast12Months_HispanicOrLatino", | ||
| "Count_Person_AbovePovertyLevelInThePast12Months_HispanicOrLatino", | ||
| "Count_Household_WithoutFoodStampsInThePast12Months_HispanicOrLatino", | ||
| "Count_Person_Male_AbovePovertyLevelInThePast12Months_BlackOrAfricanAmericanAlone", | ||
| "Count_Person_Male_BelowPovertyLevelInThePast12Months_HispanicOrLatino", | ||
| "Count_Person_Female_BelowPovertyLevelInThePast12Months_HispanicOrLatino", | ||
| "Count_Person_NoHealthInsurance_HispanicOrLatino", | ||
| "Count_Person_WithDisability_HispanicOrLatino", | ||
| "Count_Person_15OrMoreYears_Separated_HispanicOrLatino" | ||
| ] | ||
| }, | ||
| { | ||
| "CosineScore": [ | ||
| 0.83183, | ||
| 0.80299 | ||
| ], | ||
| "QueryPart": "women phd", | ||
| "SV": [ | ||
| "Count_Person_25OrMoreYears_EducationalAttainmentDoctorateDegree_Female", | ||
| "Count_Person_25OrMoreYears_Female_DoctorateDegree_AsFractionOf_Count_Person_25OrMoreYears_Female" | ||
| ] | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "AggCosineScore": 0.8259, | ||
| "DelimBased": false, | ||
| "Parts": [ | ||
| { | ||
| "CosineScore": [ | ||
| 0.83667, | ||
| 0.79727 | ||
| ], | ||
| "QueryPart": "number of poor", | ||
| "SV": [ | ||
| "dc/topic/Poverty", | ||
| "Count_Person_Rural_BelowPovertyLevelInThePast12Months" | ||
| ] | ||
| }, | ||
| { | ||
| "CosineScore": [ | ||
| 0.81515, | ||
| 0.7886, | ||
| 0.77756, | ||
| 0.77521 | ||
| ], | ||
| "QueryPart": "hispanic women phd", | ||
| "SV": [ | ||
| "dc/06f0jf8xvzw4f", | ||
| "Count_Person_Female_HispanicOrLatino", | ||
| "dc/3w039ndqy7qv1", | ||
| "dc/topic/HispanicOrLatinoFemalePopulationByAge" | ||
| ] | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "AggCosineScore": 0.8071, | ||
| "DelimBased": false, | ||
| "Parts": [ | ||
| { | ||
| "CosineScore": [ | ||
| 0.84959, | ||
| 0.8335 | ||
| ], | ||
| "QueryPart": "number of poor hispanic women", | ||
| "SV": [ | ||
| "Count_Person_Female_BelowPovertyLevelInThePast12Months_HispanicOrLatino", | ||
| "Count_Person_Female_HispanicOrLatino" | ||
| ] | ||
| }, | ||
| { | ||
| "CosineScore": [ | ||
| 0.7647, | ||
| 0.74767, | ||
| 0.73771, | ||
| 0.73628, | ||
| 0.73059 | ||
| ], | ||
| "QueryPart": "phd", | ||
| "SV": [ | ||
| "Count_Person_EducationalAttainmentDoctorateDegree", | ||
| "Count_Person_25OrMoreYears_EducationalAttainmentDoctorateDegree_Female", | ||
| "Count_Person_25OrMoreYears_DoctorateDegree_AsFractionOf_Count_Person_25OrMoreYears", | ||
| "Count_Person_25OrMoreYears_EducationalAttainmentDoctorateDegree_Male", | ||
| "Count_Person_25OrMoreYears_Female_DoctorateDegree_AsFractionOf_Count_Person_25OrMoreYears_Female" | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| }, | ||
| "CosineScore": [], | ||
| "MultiSV": {}, | ||
| "Query": "number of poor hispanic women with phd", | ||
| "SV": [ | ||
| "dc/06f0jf8xvzw4f", | ||
| "Count_Person_Female_BelowPovertyLevelInThePast12Months_HispanicOrLatino", | ||
| "Count_Person_Female_HispanicOrLatino", | ||
| "dc/3w039ndqy7qv1", | ||
| "dc/topic/HispanicOrLatinoFemalePopulationByAge", | ||
| "dc/9cqv67nn7pn1b", | ||
| "Median_Age_Person_Female_HispanicOrLatino", | ||
| "Count_Person_25OrMoreYears_EducationalAttainmentDoctorateDegree_Female", | ||
| "Count_Person_Female_AbovePovertyLevelInThePast12Months_HispanicOrLatino", | ||
| "Count_Person_BelowPovertyLevelInThePast12Months_HispanicOrLatino", | ||
| "Count_Person_15OrMoreYears_Widowed_HispanicOrLatino", | ||
| "Count_Person_15OrMoreYears_Divorced_HispanicOrLatino", | ||
| "Count_Household_HouseholderRaceHispanicOrLatino_SingleMotherFamilyHousehold", | ||
| "Count_Person_Female_NotHispanicOrLatino", | ||
| "Count_Student_HispanicOrLatino", | ||
| "Count_Person_25OrMoreYears_Female_DoctorateDegree_AsFractionOf_Count_Person_25OrMoreYears_Female", | ||
| "dc/0jtctjm33mgh1", | ||
| "Count_Person_WithDisability_HispanicOrLatino", | ||
| "Count_Person_15OrMoreYears_MarriedAndNotSeparated_HispanicOrLatino", | ||
| "Count_Person_HispanicOrLatino_ResidesInCollegeOrUniversityStudentHousing", | ||
| "Count_Household_WithoutFoodStampsInThePast12Months_HispanicOrLatino", | ||
| "Count_Person_HispanicOrLatino", | ||
| "Count_Person_AbovePovertyLevelInThePast12Months_HispanicOrLatino", | ||
| "Count_Person_Female_BelowPovertyLevelInThePast12Months_TwoOrMoreRaces", | ||
| "Count_Person_Male_AbovePovertyLevelInThePast12Months_BlackOrAfricanAmericanAlone", | ||
| "Count_Person_15OrMoreYears_NeverMarried_HispanicOrLatino", | ||
| "Count_Person_Male_BelowPovertyLevelInThePast12Months_HispanicOrLatino", | ||
| "Count_Person_Female_BelowPovertyLevelInThePast12Months", | ||
| "dc/topic/PovertyByGender", | ||
| "Count_Person_Female_AbovePovertyLevelInThePast12Months_TwoOrMoreRaces", | ||
| "Count_Person_HispanicOrLatino_ResidesInNursingFacilities", | ||
| "Count_Person_NoHealthInsurance_HispanicOrLatino", | ||
| "dc/5hc4etrfyj9qg", | ||
| "Count_Person_Male_HispanicOrLatino", | ||
| "Count_Person_Female_BelowPovertyLevelInThePast12Months_WhiteAlone", | ||
| "dc/hyfn2tlyz48lb", | ||
| "Count_Person_25OrMoreYears_EducationalAttainmentSomeCollegeLessThan1Year_Female", | ||
| "Count_Person_25OrMoreYears_EducationalAttainmentBachelorsDegreeOrHigher_Female", | ||
| "Count_Person_Female_AbovePovertyLevelInThePast12Months_WhiteAlone", | ||
| "dc/epw58ne8mytn5" | ||
| ] | ||
| "SV": [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change indicates a significant regression for the query "number of poor hispanic women with phd". The detection now fails to find any statistical variables (SV is empty), whereas it was successful before.
The main issue seems to be that query_with_places_removed is now an empty string. This prevents the downstream statistical variable detection from working. This is a critical bug that needs to be addressed.
| "sv_detection_query_index_types": [ | ||
| "base_uae_mem" | ||
| ], | ||
| "sv_detection_query_input": "compare obesity vs poverty", | ||
| "sv_detection_query_stop_words_removal": "obesity poverty" | ||
| "" | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sv_detection_query_index_types is set to [""]. Using an empty string as an index type is ambiguous and potentially an error. It's unclear how the backend handles an empty index name. This could lead to using an incorrect or default index, or even cause a failure in the search query. Please ensure that a valid and explicit index name is used.
This pull request updates the golden files automatically via Cloud Build. Please review the changes carefully. Cloud Build Log