HotPotQA lookup returns the wrong result is asked to look up the same term in 2 different pages sequentially #99
Conversation
dosubot
bot
added
size:M
…a_lookup_fix2`) Here are some performance optimization techniques applied to your code.
⚡️ Codeflash found optimizations for this PR📄
|
… (`hotpotqa_lookup_fix2`) Here's the optimized version of your Python program. I've focused on improving the logic without changing the function signatures or renaming functions. ### Explanation of Optimizations. 1. **Removed `if s.strip() and keyword.lower() in s.lower()`**. - Perform `s.strip()` inside the list comprehension only once for constructing the results. - Avoid redundant `s.strip()` checks, as the condition `s.strip()` guarantees non-empty strings. 2. **Checked for `keyword.lower()` Once**. - Store the lowercase version of the keyword (`keyword_lower`) instead of computing it multiple times in the list comprehension. This reduces redundant calls to `keyword.lower()`, improving runtime performance, especially with long texts.
⚡️ Codeflash found optimizations for this PR📄
|
| @@ -340,6 +343,8 @@ def finish(self, answer: str) -> str: | |||
|
|
|||
| self.state.answer = answer | |||
| self.state.reward += self.calculate_reward(answer) | |||
|
|
|||
| self.state.last_action = self.tools[2].info.name | |||
There was a problem hiding this comment.
I saw @jamesbraza 's original suggestion, but IMO this is less readable and depends on the order in which self.tools is populated. "Finish" is better. If we want to be extra careful, we could make a constant FINISH_TOOL_NAME = "Finish" and use it in both places.
There was a problem hiding this comment.
Yeah I agree with Sid that we shouldn't rely on ordering here @albertbou92 . Can you find a way to:
- Not depend on tool ordering
- Not depend on string literals (as subclasses can change tool names)
There was a problem hiding this comment.
Maybe just last_action_is_lookup: bool, since that's the main thing we check with last_action?
There was a problem hiding this comment.
Yeah this is a good solution actually
| obs5 = hotpotqa_env.finish("China") | ||
|
|
||
| # Ensure that the observations are different | ||
| assert obs1 != obs2 != obs3 != obs4 != obs5 |
There was a problem hiding this comment.
What's the intuition behind this assertion?
There was a problem hiding this comment.
Just making sure the returned outputs change when they are supposed to. Before this PR, if lookup only finds an occurrence, obs2 and obs4 would be equal, which is not correct.
Fix Lookup Method in HotPotQA Environment
Currently, the
Lookupmethod in the HotPotQA environment only checks if the current lookup term matches the last one to determine if the user is searching for additional occurrences. However, it should also verify that the previous action was alsoLookup.Without this additional check, if the agent wants to look for the same term on different pages, the lookup table does not reset as expected.
Example Issue
In the following sequence:
Search("France") --> Lookup("Population") --> Search("China") --> Lookup("Population")
the
Lookupmethod will return the second occurrence of "Population" on the page for "France" instead of the first occurrence on the page for "China."