[On hold] [Fixit] Place population in query context - #6583
Conversation
…displayed rather than the first overall candidate and clean up demo test data configurations
…ation test reference.
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 addresses an issue where place context was lost during query sequences in the Data Commons demo. Previously, the system assumed the first-ranked candidate would always be valid; however, if that candidate failed existence checks, subsequent valid candidates were rendered without updating the global answer places. The fix ensures that the first valid candidate encountered populates the answer context, maintaining continuity for follow-up queries. 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 the 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 counterproductive. 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
|
![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 updates the logic for setting answerPlaces across several fulfillment modules, including comparison, single-variable filtering, ranking, and time-delta calculations. Instead of restricting the population of answer places to the top-ranked chart (rank == 0), the code now checks if state.uttr.answerPlaces has not yet been set. This ensures that answer places are correctly populated when first encountered. Additionally, integration tests and their corresponding golden chart configurations have been updated to align with these changes. No review comments were provided, so there is no feedback to evaluate.
| # We only do this if this is the first chart, user requested ranking | ||
| # result and this is for 1 SV. | ||
| if (rank == 0 and len(chart_vars.svs) == 1 and | ||
| # We only do this if answer places have not been set yet, user requested |
There was a problem hiding this comment.
We may need a bit more context for this one. May be good to ask this in the channel to understand if there exists special requirement for rank_across_places
| sv_place_facet=sv_place_facet) | ||
|
|
||
| if rank == 0 and field == 'abs' and ranked_places: | ||
| if not state.uttr.answerPlaces and ranked_places: |
There was a problem hiding this comment.
do we need the field == 'abs'?
There was a problem hiding this comment.
Between abs, pct and pc, "abs" will always be processed first, so if there is an "abs" candidate, it will take precedence and the functionality will remain unchanged. If there isn't an "abs" candidate, then the functionality does change, but in a way sustains the fallback that we are aiming for overall with this PR (i.e., a non "abs" result that contains sufficient data can be used to propagate places).
So the removal of the 'abs' guard does align with the core philosophy of this PR (moving away from slot-based propagation of places).
shixiao-coder
left a comment
shixiao-coder
left a comment
There was a problem hiding this comment.
This is fine with me, may want to double check within the Experience team chat
nick-nlb
changed the title
## Related PR [6583](#6583) ## Description The place context was being lost during the `/demo` test sequence (the electrification demo). Details of the cause of this context loss are described in the above related PR ([6583](#6583)). That PR implements a fix to the root of the issue that causes the place context to be lost. However, further consideration is required to ensure that the fix in that PR does not cause unintended downstream changes. In the meantime, this PR fixes the sequence itself by altering the wording of the query so that the paths that result in context lost are not triggered. ## Testing You cannot test this sequence from the `/demo` link, because this will not be updated until after the GCS bucket that holds the live redirect file is updated (see `server/routes/redirects/README.md`). However you can test it both locally or in production by going to the relevant redirected link directly: [Local sequence: Fixed](http://localhost:8080/explore/#aq=Which+countries+in+Africa+have+had+the+greatest+increase+in+electricity+access?___How+has+poverty+changed+over+time+in+these+countries?___How+has+life+expectancy+increased+in+these+places?___How+has+the+GDP+grown?___What+is+the+greenhouse+gas+emissions+from+these+places?___How+do+these+places+compare+with+the+US+and+Germany?&ae=1) [Production sequence: Fixed](https://datacommons.org/explore/#aq=Which+countries+in+Africa+have+had+the+greatest+increase+in+electricity+access?___How+has+poverty+changed+over+time+in+these+countries?___How+has+life+expectancy+increased+in+these+places?___How+has+the+GDP+grown?___What+is+the+greenhouse+gas+emissions+from+these+places?___How+do+these+places+compare+with+the+US+and+Germany?&ae=1) Compare to the current production demo, where context becomes visible at the third query: [Production sequence: Not fixed](https://datacommons.org/demo) ## Post Merging Once this is merged (and we are are during business hours), we can run the script described at `server/routes/redirects/README.md` to push the updated sequence to the bucket and make it live.
2 participants
Issue
b/546625182
Related PR
6585 - this PR is a mitigation method to solve the query sequence itself
Description
It was recently discovered that the demo sequence (https://datacommons.org/demo) is broken. Between the second and third queries, the place context is lost (causing the sequence to lose the initial tracked countries and fall back to Earth).
The context becomes lost because of how the fulfillment handlers propagated the place list from query to query. During the query fulfillment process, SV candidates are detected and ranked. The fulfillment loop then iterates through each candidate, passing them to a handler in which the candidate undergoes existence checks. If the data is too sparse, the candidate is discarded.
Before this PR, handlers guarded the population of state.uttr.answerPlaces with a
rank == 0criteria, likely under the assumption that index 0 would always represent the rendered result. However, when the 0th-ranked candidate failed existence checks and was discarded, subsequent candidates (evaluated with rank >= 1) successfully generated charts but never populated state.uttr.answerPlaces.Because of this the utterance finished with an empty answerPlaces list, causing follow-up queries referencing "these places" to fall back to Earth.
Solution
This PR updates the guard to
answerPlacespopulation by populating the answerPlaces list with the highest ranked candidate that passes the data existence checks supplies places toanswerPlaces, rather than just the very first candidate (that may have been discarded).Why now?
As mentioned above, this problem was surfaced through the
/demoquery sequence, which was previously working. The code that populatesanswerPlacesexisted in its current form at a time when the demo was working. It is likely that recent changes to data and embeddings (and thus the candidate list) has changed. While the demo sequence may have worked in the past, other sequences would have failed (i.e., this is not a new problem).Notes
This PR regenerates goldens (the goldens associated with sequences change with this PR as expected). It also fixes a spelling error in one of the searches.
Testing
You can test the fix by running the demo sequence locally in
masterand then in this PR (or alternatively, in this PR and in autopush). In this PR, the place context will remain until the end of the sequence. In master, the place context is lost.Related PR
There is an unrelated error in the demo itself, where, when the next query in the sequence is typed in, it changes not only the query in the search, but the query as it is rendered in the search results themselves. This will be fixed in a follow-up PR.