Use newest PMAT (Metaculus fixes, supported_markets property) #529
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe pull request introduces several modifications across multiple agent classes within the prediction market agent module. Key changes include the addition of a 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: 3
🧹 Outside diff range and nitpick comments (5)
prediction_market_agent/agents/specialized_agent/deploy.py (1)
Line range hint
35-49: Consider adding market type verification or removing unused parameter.The
get_marketsmethod accepts amarket_typeparameter but doesn't verify it againstsupported_markets. This could lead to confusion as the method signature suggests support for different market types.Consider one of these alternatives:
# Option 1: Add verification def get_markets( self, market_type: MarketType, limit: int = MAX_AVAILABLE_MARKETS, sort_by: SortBy = SortBy.CLOSING_SOONEST, filter_by: FilterBy = FilterBy.OPEN, ) -> t.Sequence[OmenAgentMarket]: + if market_type not in self.supported_markets: + raise ValueError(f"Unsupported market type: {market_type}") available_markets = [ OmenAgentMarket.from_data_model(m) for m in OmenSubgraphHandler().get_omen_binary_markets_simple( limit=limit, sort_by=sort_by, filter_by=filter_by, creator_in=SPECIALIZED_FOR_MARKET_CREATORS, ) ]# Option 2: Remove unused parameter -def get_markets( - self, - market_type: MarketType, - limit: int = MAX_AVAILABLE_MARKETS, - sort_by: SortBy = SortBy.CLOSING_SOONEST, - filter_by: FilterBy = FilterBy.OPEN, -) -> t.Sequence[OmenAgentMarket]: +def get_markets( + self, + limit: int = MAX_AVAILABLE_MARKETS, + sort_by: SortBy = SortBy.CLOSING_SOONEST, + filter_by: FilterBy = FilterBy.OPEN, +) -> t.Sequence[OmenAgentMarket]:prediction_market_agent/agents/invalid_agent/deploy.py (1)
14-16: Consider formatting the docstring according to Python conventions.While the content is informative, consider restructuring it to follow standard Python docstring conventions.
- """This agent works only on Omen. - Because on Omen, after market is resolved as invalid, outcome tokens are worth equally, which means one can be profitable by buying the cheapest token. - Also the function to mark invalid questions is based on Omen-resolution rules.""" + """Agent specialized for detecting and profiting from invalid Omen markets. + + This agent operates exclusively on Omen markets, leveraging their specific + resolution mechanics where invalid markets result in equal-value outcome tokens. + The strategy involves buying the cheapest token, as all tokens will have equal + value upon invalid resolution. + + Note: + The invalid market detection is specifically tailored to Omen's resolution rules. + """prediction_market_agent/agents/metaculus_agent/deploy.py (3)
58-62: Simplify the conditional return statement inverify_market.You can simplify the logic in the
verify_marketmethod by returning the condition directly, as suggested by the static analysis hint (SIM103).Current code:
if not self.repeat_predictions and market.have_predicted: return False # Otherwise all markets on Metaculus are fine. return TrueSuggested change:
-def verify_market(self, market_type: MarketType, market: AgentMarket) -> bool: - if not self.repeat_predictions and market.have_predicted: - return False - return True +def verify_market(self, market_type: MarketType, market: AgentMarket) -> bool: + return self.repeat_predictions or not market.have_predictedThis refactoring makes the code more concise and improves readability.
🧰 Tools
🪛 Ruff
58-62: Return the negated condition directly
(SIM103)
53-55: Avoid usingassertstatements for type checking inverify_market.Using
assertstatements for type checking can be unsafe because assertions can be disabled at runtime with optimization flags (-O). In theverify_marketmethod:assert isinstance( market, MetaculusAgentMarket ), "Just making mypy happy. It's true thanks to the check in the `run` method via `supported_markets`."Consider using explicit type casting to inform the type checker without affecting runtime behavior.
Suggested change:
market = typing.cast(MetaculusAgentMarket, market)This approach satisfies type checking requirements without relying on assertions.
65-67: Use explicit type casting instead ofassertfor type checking inanswer_binary_market.Similarly, in the
answer_binary_marketmethod:assert isinstance( market, MetaculusAgentMarket ), "Just making mypy happy. It's true thanks to the check in the `run` method via `supported_markets`."Replace the
assertstatement with an explicit type cast to ensure type safety without impacting runtime behavior.Suggested change:
market = typing.cast(MetaculusAgentMarket, market)
📜 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/invalid_agent/deploy.py (1 hunks)
- prediction_market_agent/agents/known_outcome_agent/deploy.py (2 hunks)
- prediction_market_agent/agents/metaculus_agent/deploy.py (2 hunks)
- prediction_market_agent/agents/specialized_agent/deploy.py (1 hunks)
🧰 Additional context used
🪛 Ruff
prediction_market_agent/agents/metaculus_agent/deploy.py
58-62: Return the negated condition directly
(SIM103)
🔇 Additional comments (5)
prediction_market_agent/agents/specialized_agent/deploy.py (2)
33-33: LGTM! The supported_markets declaration accurately reflects the agent's capabilities.The attribute correctly specifies OMEN market support, which aligns with the implementation using OmenAgentMarket and OmenSubgraphHandler.
Line range hint
33-49: Verify consistent implementation across other agents.Let's check if other agents follow the same pattern of market type support declaration and handling.
✅ Verification successful
Let me gather more information to verify the market type support implementation more thoroughly.
This will help us understand:
- All agent implementations in the codebase
- How MarketType is used across the codebase
- Different market handler implementations
Based on the gathered information, I can now provide a final response regarding the consistency of market type support implementation.
Market type support implementation is consistent with the codebase design
The implementation in
specialized_agent/deploy.pyfollows the established pattern in the codebase where:
- Market type support is explicitly declared via
supported_marketsclass attribute- The
get_marketsmethod acceptsmarket_typeparameter consistently- The implementation correctly handles OMEN markets as declared in
supported_marketsKey findings from the codebase:
- Other agents like
DeployableKnownOutcomeAgentfollow the same pattern withsupported_markets = [MarketType.OMEN]- Market type verification is consistently implemented across agents (e.g., in
verify_marketmethods)- The
MarketTypeenum is used consistently throughout the codebase for type safety🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check market type support implementation across agents # Expected results: # 1. All agent classes should declare supported_markets # 2. Consistent handling of market_type parameter in get_markets methods # Check for supported_markets declarations echo "Checking supported_markets declarations:" ast-grep --pattern 'class $_ { $$$ supported_markets: $_ = $_ $$$ }' # Check for get_markets method implementations echo -e "\nChecking get_markets implementations:" ast-grep --pattern 'def get_markets( self, market_type: MarketType, $$$ ) -> $_: $$$'Length of output: 2084
Script:
#!/bin/bash # Let's search for all agent classes and their market type related code # Find all agent class definitions echo "=== Agent Class Definitions ===" rg "class.*Agent.*:" -A 5 # Check for MarketType imports and usage echo -e "\n=== MarketType Imports and Usage ===" rg "MarketType" # Check for market-related handler implementations echo -e "\n=== Market Handler Implementations ===" rg "get_.*_markets" -A 2Length of output: 42396
prediction_market_agent/agents/invalid_agent/deploy.py (2)
19-19: LGTM! Clear declaration of market type support.The explicit declaration of supported markets enhances code clarity and aligns with the agent's specialized purpose.
Line range hint
21-31: Verify market type validation in parent class.The
verify_marketmethod no longer explicitly checks the market type. Let's verify that the parent classDeployableTraderAgentproperly handles market type validation using the newsupported_marketsattribute.prediction_market_agent/agents/known_outcome_agent/deploy.py (1)
23-23: LGTM! Explicit market type support improves clarity.The addition of
supported_marketsmakes the agent's capabilities explicit and aligns with the standardized approach across other agents.
| assert isinstance( | ||
| market, OmenAgentMarket | ||
| ), "It's true thanks to supported_markets property, this just makes mypy happy." |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using runtime type checking instead of assertions.
While the assertion helps with type checking, it has some limitations:
- Assertions can be disabled with Python's -O flag
- The error message could be more descriptive for debugging
Consider this alternative implementation that provides better runtime safety:
- assert isinstance(
- market, OmenAgentMarket
- ), "It's true thanks to supported_markets property, this just makes mypy happy."
+ if not isinstance(market, OmenAgentMarket):
+ raise TypeError(
+ f"Expected OmenAgentMarket but got {type(market).__name__}. "
+ f"This agent only supports {self.supported_markets}"
+ )This approach:
- Provides explicit runtime type checking
- Gives more informative error messages
- Cannot be disabled by Python flags
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert isinstance( | |
| market, OmenAgentMarket | |
| ), "It's true thanks to supported_markets property, this just makes mypy happy." | |
| if not isinstance(market, OmenAgentMarket): | |
| raise TypeError( | |
| f"Expected OmenAgentMarket but got {type(market).__name__}. " | |
| f"This agent only supports {self.supported_markets}" | |
| ) |
| ) | ||
|
|
||
| def get_markets(self) -> Sequence[AgentMarket]: # type: ignore # TODO: Needs to be decided in https://github.com/gnosis/prediction-market-agent/pull/511#discussion_r1810034688 and then I'll implement it here. |
There was a problem hiding this comment.
Address the TODO comment and type ignore in get_markets.
In the get_markets method, there's a TODO comment and a # type: ignore directive:
def get_markets(self) -> Sequence[AgentMarket]: # type: ignore # TODO: Needs to be decided in https://github.com/gnosis/prediction-market-agent/pull/511#discussion_r1810034688 and then I'll implement it here.Please resolve the TODO by implementing the necessary changes once the discussion in PR #511 is concluded. After that, consider removing the # type: ignore to ensure proper type checking.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
prediction_market_agent/agents/metaculus_agent/deploy.py (2)
27-34: Add docstring explaining the sys.maxsize usage.Consider adding a class-level docstring explaining why
sys.maxsizeis used forbet_on_n_markets_per_run. While the inline comment provides some context, a more detailed explanation in the class docstring would be helpful for maintainers.class DeployableMetaculusBotTournamentAgent(DeployablePredictionAgent): + """Metaculus tournament agent that processes all available markets in a single run. + + The agent uses sys.maxsize for bet_on_n_markets_per_run since Metaculus predictions + are free and we want to process all available markets in one execution. + """
36-40: Enhance documentation about agent selection criteria.The comment about using this agent due to "lowest
p_yes mse" could be more specific. Consider adding the actual MSE value and when this evaluation was performed to help future maintainers understand the selection criteria.
📜 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 (1)
- prediction_market_agent/agents/metaculus_agent/deploy.py (1 hunks)
🧰 Additional context used
🪛 Ruff
prediction_market_agent/agents/metaculus_agent/deploy.py
21-21:
prediction_market_agent.utils.DEFAULT_OPENAI_MODELimported but unusedRemove unused import:
prediction_market_agent.utils.DEFAULT_OPENAI_MODEL(F401)
59-63: Return the negated condition directly
(SIM103)
| from prediction_market_agent.agents.think_thoroughly_agent.think_thoroughly_agent import ( | ||
| ThinkThoroughlyWithItsOwnResearch, | ||
| from prediction_market_agent.agents.prophet_agent.deploy import ( | ||
| DeployablePredictionProphetGPTo1PreviewAgent, | ||
| ) | ||
| from prediction_market_agent.utils import DEFAULT_OPENAI_MODEL |
There was a problem hiding this comment.
Remove unused import.
The DEFAULT_OPENAI_MODEL import is not used in this file.
-from prediction_market_agent.utils import DEFAULT_OPENAI_MODEL📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from prediction_market_agent.utils import DEFAULT_OPENAI_MODEL |
🧰 Tools
🪛 Ruff
21-21:
prediction_market_agent.utils.DEFAULT_OPENAI_MODELimported but unusedRemove unused import:
prediction_market_agent.utils.DEFAULT_OPENAI_MODEL(F401)
| logger.info(f"Answering market {market.id}, question: {market.question}") | ||
| answer: ProbabilisticAnswer | None | ||
| if not self.dummy_prediction: | ||
| full_question = f"""Question: {market.question} |
There was a problem hiding this comment.
The previous Metaculus tournament wasn't tracked in Langfuse, so there is not much to debug.
Thanks to this refactoring it will be tracked now, example: https://cloud.langfuse.com/project/clv2hvvyw0006z9uchz6u1irw/traces/15a2dd9d-20e7-4ef3-9235-c41050064661?observation=3c1861b6-8866-4e27-8182-4196d091e08e
I thought this is a fine way to add additional fields to the existing agents -- if it turns out that it works, we could make it officially like this. Something like the full_question property on AgentMarket.
| answer = ProbabilisticAnswer( | ||
| p_yes=Probability(0.5), | ||
| reasoning="Just a test.", | ||
| confidence=0.5, | ||
| ) |
There was a problem hiding this comment.
why not return None?
There was a problem hiding this comment.
There was a problem hiding this comment.
oh haha! it must be right then...
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
prediction_market_agent/agents/metaculus_agent/deploy.py (1)
64-82: Consider adding error handling for market attributes.While the implementation is generally good, the question construction could benefit from handling potential None values in market attributes to prevent potential runtime errors.
Consider updating the question construction with null checks:
- full_question = f"""Question: {market.question} -Question's description: {market.description} -Question's fine print: {market.fine_print} -Question's resolution criteria: {market.resolution_criteria}""" + full_question = f"""Question: {market.question or ''} +Question's description: {market.description or ''} +Question's fine print: {market.fine_print or ''} +Question's resolution criteria: {market.resolution_criteria or ''}""".strip()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- prediction_market_agent/agents/metaculus_agent/deploy.py (1 hunks)
- prediction_market_agent/agents/think_thoroughly_agent/deploy.py (1 hunks)
- scripts/agent_app.py (1 hunks)
🧰 Additional context used
🪛 Ruff
prediction_market_agent/agents/metaculus_agent/deploy.py
58-62: Return the negated condition directly
(SIM103)
🔇 Additional comments (7)
prediction_market_agent/agents/think_thoroughly_agent/deploy.py (1)
54-56: LGTM! Verify parameter usage across the codebase.The new parameters provide better granularity and control over the agent's behavior, separating concerns between trading and storage operations.
Let's verify that these new parameters are consistently used across the codebase:
prediction_market_agent/agents/metaculus_agent/deploy.py (5)
4-4: LGTM: Class inheritance and imports updated appropriately.The change from
DeployableAgenttoDeployablePredictionAgentbase class aligns with the PR objectives, and all necessary imports have been added to support the new functionality.Also applies to: 7-11, 18-19, 26-26
27-33: LGTM: Clear specification of supported markets and well-documented attributes.The
supported_marketsproperty clearly specifiesMarketType.METACULUSsupport, and the existing attributes are well-documented, particularly the explanation for usingsys.maxsizeforbet_on_n_markets_per_run.
35-40: LGTM: Well-documented agent initialization.The
loadmethod clearly initializes the appropriate agent with good documentation explaining the selection criteria based on evaluation results.
41-50: LGTM: Market retrieval implementation.The implementation correctly retrieves binary markets with appropriate filtering and sorting.
52-62: LGTM: Market verification logic.The implementation correctly handles market verification with appropriate type assertions and prediction repeat checks.
🧰 Tools
🪛 Ruff
58-62: Return the negated condition directly
(SIM103)
scripts/agent_app.py (1)
81-83: Verify consistent agent initialization across the codebase.Let's ensure that all agent initializations include the new parameters and handle them consistently.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Agent initialization parameters are consistent across the codebase
The verification shows that the parameters
place_trades,store_prediction, andstore_tradesare consistently handled:
- In
scripts/agent_app.py, the main initialization matches the changes- In
prediction_market_agent/agents/think_thoroughly_agent/deploy.py, the agent initialization includes the same parameters with identical values- Other agent implementations (
DeployableKnownOutcomeAgent,DeployableThinkThoroughlyAgentBase) inherit from base classes and don't override these parameters- The
predictfunction implementations across different agents don't rely on these parameters🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for agent initializations and predict function calls # Test 1: Search for agent initializations to verify parameter consistency echo "Checking agent initializations..." rg -A 5 "AgentClass\(" rg -A 5 "DeployableThinkThoroughlyAgent\(" rg -A 5 "DeployableKnownOutcomeAgent\(" # Test 2: Search for predict function calls to verify parameter passing echo "Checking predict function calls..." rg -A 5 "def predict\(" rg -A 5 "predict\("Length of output: 9303
| place_trades=False, | ||
| store_prediction=False, | ||
| store_trades=False, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making agent behavior configurable.
Currently, all new parameters (place_trades, store_prediction, store_trades) are hardcoded to False. This might limit the agent's functionality in the Streamlit app.
Consider adding these as configurable options in the Streamlit UI:
agent = AgentClass(
- place_trades=False,
- store_prediction=False,
- store_trades=False,
+ place_trades=st.checkbox("Place trades", value=False),
+ store_prediction=st.checkbox("Store prediction", value=False),
+ store_trades=st.checkbox("Store trades", value=False),
enable_langfuse=enable_langfuse,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| place_trades=False, | |
| store_prediction=False, | |
| store_trades=False, | |
| place_trades=st.checkbox("Place trades", value=False), | |
| store_prediction=st.checkbox("Store prediction", value=False), | |
| store_trades=st.checkbox("Store trades", value=False), |
Function signature doesn't match the implementation.
The function signature of predict is missing the new parameters (place_trades, store_prediction, store_trades) that are being passed to the agent initialization. This could lead to confusion and maintenance issues.
Update the function signature to match the implementation:
def predict(
AgentClass: SupportedAgentType,
market_source: MarketType,
market: AgentMarket,
skip_market_verification: bool,
enable_langfuse: bool,
+ place_trades: bool = False,
+ store_prediction: bool = False,
+ store_trades: bool = False,
) -> None:Committable suggestion was skipped due to low confidence.
No description provided.