Fix challenging agents (replicator, challenger) #510
Conversation
| + timedelta(minutes=30), | ||
| ) + get_omen_binary_markets_common_filters( | ||
| # And also markets without any answer at all yet. | ||
| question_with_answers=False, |
There was a problem hiding this comment.
I didn't realise before that if I use question_finalized_after, it won't include questions without any answer at all (where it's null)
There was a problem hiding this comment.
Effectivelly what you are doing with the partial is an OR filter, since you want (in this code snippet) the questions that finalize after now() + 30min OR questions without answers.
For this, I find querying the subgraph using OR a lot cleaner - see code snippet below (example query).
{
questions(first: 5,
where: {
or: [{
id: "0x001dd5a9194948a903c0e3b624eb0d458cd23d828f29a57f517c858d7cf71f76"
},
{id : "0x0005ef01269fbb37aba4cc16f518138d7d8bf5f4937a5572c294c2e62f0cf670"}
],
}) {
id
templateId
data
title
}
}
What do you think?
There was a problem hiding this comment.
using OR a lot cleaner
It is clean in the simple example query you provided, but how do you suggest integrating or functionality into OmenSubgraphHandler? With any integration that I can imagine, it's in the end easier to just do it like I do it here.
finalized_aftershouldn't befinalized_after OR without_answers, because sometimes we might need to really query onlyfinalized_aftermarkets- I didn't want to introduce
finalize_after_or_nullfilter, because that we would have to haveor_nullvariant of many other arguments as well
There was a problem hiding this comment.
I agree partial here is simple and works, but what I was thinking is something along these lines (inside the subgraph handler) if the use case becomes more complicated:
where_stm1 = -> build with params 1
where_stm2 -> build with params2
where_final = {"or": [where_stms1, where_stms2]}
The added benefit here is to execute just 1 query, whereas in yours we execute 2 queries.
There was a problem hiding this comment.
But happy here with partial :)
There was a problem hiding this comment.
That will need some fancy refactoring of OmenSubgraphHandler, but I agree it could be nice!
I'm not sure how it would end up in the end, but if you are up to exploring it, it sounds good to me.
to execute just 1 query, whereas in yours we execute 2 queries.
That's a good benefit out of it.
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request involve updates to several Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
prediction_market_agent/agents/replicate_to_omen_agent/omen_resolve_replicated.py (1)
56-71: Excellent refactoring for improved clarity and coverageThe changes in this section significantly improve the code:
- The introduction of
get_omen_binary_markets_common_filtersas a partial function is an excellent way to reduce code duplication and improve maintainability.- Updating parameter names (e.g.,
opened_beforetoquestion_opened_before) enhances clarity.- Adding an additional query for markets without any answers ensures more comprehensive coverage.
These changes make the code more readable and flexible. Great job!
Consider renaming
created_opened_marketsto something likemarkets_to_processorcandidate_marketsto better reflect its expanded content (now including markets without answers).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
poetry.lockis excluded by!**/*.lock,!**/*.lockpyproject.tomlis excluded by!**/*.toml
📒 Files selected for processing (4)
- prediction_market_agent/agents/ofvchallenger_agent/deploy.py (2 hunks)
- prediction_market_agent/agents/omen_cleaner_agent/deploy.py (1 hunks)
- prediction_market_agent/agents/replicate_to_omen_agent/omen_replicate.py (1 hunks)
- prediction_market_agent/agents/replicate_to_omen_agent/omen_resolve_replicated.py (2 hunks)
🧰 Additional context used
🔇 Additional comments (7)
prediction_market_agent/agents/replicate_to_omen_agent/omen_resolve_replicated.py (4)
2-2: LGTM: Good use offunctools.partialThe addition of
from functools import partialis a good choice. It suggests that you're planning to use partial functions to create reusable function objects with pre-filled arguments, which can lead to cleaner and more maintainable code.
56-56: Improved type safety with return type annotationThe addition of the
-> FinalizeAndResolveResultreturn type annotation is a great improvement. It enhances type safety and makes the function's contract more explicit, which aids in code understanding and maintenance.
Line range hint
180-180: Enhanced type safety with ClaimResult return typeThe addition of the
-> ClaimResultreturn type annotation is a positive change. It improves type safety and makes the function's output more explicit, which is beneficial for code clarity and maintenance.
Line range hint
1-224: Overall excellent improvements to the codebaseThe changes in this file demonstrate a thoughtful approach to improving the code:
- The introduction of partial functions reduces code duplication.
- Enhanced type annotations improve type safety and code clarity.
- Expanded market retrieval logic ensures more comprehensive coverage.
- Improved parameter naming enhances code readability.
These changes align well with the PR objectives of fixing challenging agents (replicator, challenger). They should lead to more robust and maintainable code.
Great work on these improvements!
prediction_market_agent/agents/ofvchallenger_agent/deploy.py (2)
2-2: LGTM: Import ofpartialfunction.The addition of
from functools import partialis appropriate given the upcoming changes in the code that utilize this function.
72-86: Excellent refactoring usingpartialfunction.The changes in the
challengemethod significantly improve code readability and efficiency:
- The use of
partialto createget_omen_binary_markets_common_filtersreduces code duplication and simplifies parameter passing.- The updated logic now correctly handles both markets that will be finalized after a 30-minute buffer and markets without any answers, addressing the issue mentioned in previous review comments.
These changes make the code more maintainable and aligned with the intended functionality.
prediction_market_agent/agents/replicate_to_omen_agent/omen_replicate.py (1)
208-208: LGTM! Verify consistency across the codebase.The parameter name change from
opened_beforetoquestion_opened_beforein theget_omen_binary_marketsmethod call improves clarity. It better specifies that the filter is applied to the question's opening time.To ensure consistency, please run the following script to check if this change has been applied uniformly across the codebase:
This will help identify any places where the parameter name might need to be updated for consistency.
✅ Verification successful
Verified consistency across the codebase.
All instances of
opened_beforehave been successfully renamed toquestion_opened_beforeinget_omen_binary_marketsmethod calls.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of 'opened_before' in get_omen_binary_markets calls # Search for any remaining instances of 'opened_before' in get_omen_binary_markets calls rg --type python 'get_omen_binary_markets.*opened_before' # Search for all instances of 'get_omen_binary_markets' to compare rg --type python 'get_omen_binary_markets'Length of output: 164
Script:
#!/bin/bash # Description: Check for any remaining instances of 'opened_before' in get_omen_binary_markets calls # Search for any remaining instances of 'opened_before' in get_omen_binary_markets calls rg -g '*.py' 'get_omen_binary_markets.*opened_before' # Search for all instances of 'get_omen_binary_markets' to compare rg -g '*.py' 'get_omen_binary_markets'Length of output: 2310
| @@ -59,7 +59,7 @@ def resolve_finalized_markets(self, api_keys: APIKeys) -> list[HexAddress]: | |||
| market | |||
| for market in OmenSubgraphHandler().get_omen_binary_markets( | |||
| limit=None, | |||
| finalized_before=utcnow(), | |||
| question_finalized_before=utcnow(), | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent parameter naming in get_omen_binary_markets calls.
The question_finalized_before parameter was updated in deploy.py, but other calls to get_omen_binary_markets still use the old parameter name finalized_before. Please update these method calls to ensure consistent parameter naming across the codebase.
🔗 Analysis chain
Approve the parameter name change and verify consistency.
The change from finalized_before to question_finalized_before improves clarity by specifying that we're filtering based on the question's finalization time. This aligns well with the method's purpose of resolving finalized markets.
To ensure consistency across the codebase, please run the following script:
This will help confirm that the OmenSubgraphHandler class and its usage are consistent with this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of 'question_finalized_before' parameter in OmenSubgraphHandler
# Test: Search for OmenSubgraphHandler class definition and its method calls
rg --type python -A 10 'class OmenSubgraphHandler'
rg --type python 'OmenSubgraphHandler\(\)\.get_omen_binary_markets'
Length of output: 184
Script:
#!/bin/bash
# Description: Verify the usage of 'question_finalized_before' parameter in OmenSubgraphHandler
# Test: Search for OmenSubgraphHandler class definition and its method calls
rg --type py -A 10 'class OmenSubgraphHandler'
rg --type py 'OmenSubgraphHandler\(\)\.get_omen_binary_markets'
Length of output: 1431
Script:
#!/bin/bash
# Description: Verify the usage of 'question_finalized_before' parameter in all calls to 'get_omen_binary_markets'
# Search for 'get_omen_binary_markets' method calls with 'question_finalized_before' parameter
rg --type py 'get_omen_binary_markets\([^)]*question_finalized_before='
Length of output: 73
No description provided.