-
Notifications
You must be signed in to change notification settings - Fork 115
Automated Integration Test Goldens Update from CI #5930
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 #5930
Conversation
…1d6-8538-02c952939182)
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 automatically updates integration test golden files, reflecting recent changes in the backend's query processing and Statistical Variable (SV) matching algorithms. The updates demonstrate refined query interpretation, including the integration of Large Language Models for certain query types, and adjustments to how SVs are identified and scored. These changes ensure that the integration tests accurately reflect the current system behavior. 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. 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 golden files for integration tests.
The changes for the compareobesityvs.poverty test case appear to be an improvement, with better metric detection from the new LLM-based flow. However, there's a potentially problematic value in the debug logs for sv_detection_query_index_types.
More critically, the changes for the numberofpoorhispanicwomenwithphd test case show a significant regression. The new version fails to detect any statistical variables, whereas the previous version was successful. This seems to be caused by an issue in the query processing logic. This regression should be addressed.
| "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 this test case. The previous version correctly identified numerous statistical variables (SVs), but the new version finds none (SV is empty).
The root cause appears to be that query_with_places_removed is now an empty string. With an empty query, it's expected that no SVs would be matched. This suggests a bug in the upstream query processing logic that strips the entire query.
| "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 now [""]. Previously, it was ["base_uae_mem"]. Using an array with an empty string as an index type is ambiguous and could be a bug. If the intention is to not use any embeddings index, it would be clearer to use an empty array []. This might indicate an issue where the embeddings index is not being correctly passed or used in the new LLM-based detection flow.
|
This pull request updates the golden files automatically via Cloud Build. Please review the changes carefully. Cloud Build Log