Add prophet-gpt4o agent that trades on new markets #537
Conversation
WalkthroughThe changes introduce a new class, 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 (
|
evangriffiths
changed the title
![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 (2)
prediction_market_agent/agents/prophet_agent/deploy.py (2)
85-87: Optimize by reusing theAPIKeys()instanceThe
APIKeys()object is instantiated multiple times, such as in lines 86 and 96. Creating it once and reusing it can improve performance and clarity.You could initialize
APIKeys()once and store it as an instance variable:class DeployablePredictionProphetGPT4oAgentNewMarketTrader( DeployablePredictionProphetGPT4oAgent ): + def __init__(self): + super().__init__() + self.api_keys = APIKeys() relevant_news_response_cache = RelevantNewsResponseCache( - sqlalchemy_db_url=APIKeys().sqlalchemy_db_url.get_secret_value() + sqlalchemy_db_url=self.api_keys.sqlalchemy_db_url.get_secret_value() )And update line 96:
user_id = market.get_user_id( - api_keys=APIKeys() + api_keys=self.api_keys )
72-74: Simplify the class name for readabilityThe class name
DeployablePredictionProphetGPT4oAgentNewMarketTraderis quite long, which may affect readability and maintainability. Consider shortening it for clarity.Possible alternatives:
NewMarketTraderAgentProphetGPT4oNewMarketTrader
📜 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 (2)
prediction_market_agent/agents/prophet_agent/deploy.py(2 hunks)prediction_market_agent/run_agent.py(3 hunks)
🔇 Additional comments (3)
prediction_market_agent/run_agent.py (2)
39-39: LGTM! Import statement is properly placed.
The new agent import is correctly added within the prophet_agent imports block, maintaining proper organization.
75-75: LGTM! Agent integration follows the established pattern.
The new agent is properly integrated into the system by:
- Adding an enum entry in
RunnableAgent - Mapping it in the
RUNNABLE_AGENTSdictionary
This follows the file's documented pattern for adding new agents that adhere to the PMAT standard.
Also applies to: 107-107
prediction_market_agent/agents/prophet_agent/deploy.py (1)
82-82:
Verify the trade_on_markets_created_after date
The attribute trade_on_markets_created_after is set to DatetimeUTC(2024, 10, 31, 0), which is October 31, 2024. Given that the current date is October 2024, this means the agent will not trade on any markets created until that date. Please confirm if this is the intended behavior.
| """ | ||
|
|
||
| bet_on_n_markets_per_run = 20 | ||
| trade_on_markets_created_after = DatetimeUTC(2024, 10, 31, 0) |
There was a problem hiding this comment.
Why is this necessary for this agent?
There was a problem hiding this comment.
Hmm well it's not necessary, but it fits the description of betting on 'new markets, and re-evaluates them over their lifetime'. An alternative solution would be to get the agent's deployment start_time at runtime, but it's still not great because it assumes that env var (defined in the deployment code) exists
| get_markets_sort_by = SortBy.NEWEST | ||
| same_market_trade_interval = MarketLifetimeProportionalInterval(max_trades=4) | ||
| relevant_news_response_cache = RelevantNewsResponseCache( | ||
| sqlalchemy_db_url=APIKeys().sqlalchemy_db_url.get_secret_value() |
There was a problem hiding this comment.
This is initialised at import-time, it will error out if sqlalchemy_db_url is not defined in the environment, I think that is the case for some agents. (load method can be used for this)
There was a problem hiding this comment.
eeek good point, thanks!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
prediction_market_agent/agents/prophet_agent/deploy.py (3)
82-84: Consider making the deployment date configurable.The hardcoded deployment date (
2024, 10, 31) should ideally be configurable through environment variables or configuration files to allow for flexibility in different deployment environments.Consider this approach:
- trade_on_markets_created_after = DatetimeUTC(2024, 10, 31, 0) # Date of deployment + trade_on_markets_created_after = DatetimeUTC.fromisoformat( + os.getenv('TRADE_START_DATE', '2024-10-31T00:00:00+00:00') + )
84-84: Document the rationale for max_trades=4.The
max_trades=4setting inMarketLifetimeProportionalIntervalseems arbitrary. Consider adding a comment explaining why this specific value was chosen and what trade-offs it represents.
92-110: Add debug logging for market verification steps.To aid in debugging and monitoring, consider adding logging statements to track the verification process:
def verify_market(self, market_type: MarketType, market: AgentMarket) -> bool: if not super().verify_market(market_type, market): + logger.debug(f"Market {market.id} failed base verification") return False user_id = market.get_user_id(api_keys=APIKeys()) last_trade_datetime = market.get_most_recent_trade_datetime(user_id=user_id) if last_trade_datetime is None: + logger.debug(f"First trade attempt for market {market.id}") return True news = get_certified_relevant_news_since_cached( question=market.question, days_ago=(utcnow() - last_trade_datetime).days, cache=self.relevant_news_response_cache, tavily_storage=self.agent.tavily_storage, ) + logger.debug(f"Market {market.id}: Found {'new' if news else 'no new'} relevant news") return news is not None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
prediction_market_agent/agents/prophet_agent/deploy.py(2 hunks)
🔇 Additional comments (1)
prediction_market_agent/agents/prophet_agent/deploy.py (1)
6-20: LGTM: Imports are well-organized and necessary.
The new imports support the market lifetime intervals, news analysis, and caching functionality required by the new agent.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
prediction_market_agent/agents/prophet_agent/deploy.py (1)
72-79: Consider making news relevance criteria configurable.The agent's behavior regarding news-based re-evaluation could benefit from configurable criteria, such as news source reliability thresholds or keyword importance weights.
This would allow fine-tuning the agent's sensitivity to different types of news without code changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
prediction_market_agent/agents/prophet_agent/deploy.py(2 hunks)
🔇 Additional comments (2)
prediction_market_agent/agents/prophet_agent/deploy.py (2)
86-90: Add error handling for cache initialization.
This issue was previously identified in past reviews. The cache initialization could fail if the database URL is not defined or if there are connection issues.
104-110: Add error handling for news fetching.
This issue was previously identified in past reviews. The get_certified_relevant_news_since_cached call should be wrapped in error handling.
| """ | ||
|
|
||
| bet_on_n_markets_per_run = 20 | ||
| trade_on_markets_created_after = DatetimeUTC(2024, 10, 31, 0) # Date of deployment |
There was a problem hiding this comment.
Consider making the market creation date configurable.
The hardcoded deployment date (2024, 10, 31) could cause issues if the code is deployed at a different time. Consider making this configurable through environment variables or constructor parameters.
Example implementation:
- trade_on_markets_created_after = DatetimeUTC(2024, 10, 31, 0) # Date of deployment
+ trade_on_markets_created_after = None # Will be set in __init__
+
+ def __init__(self, *args, **kwargs):
+ super().__init__(*args, **kwargs)
+ # Default to current date if not specified
+ self.trade_on_markets_created_after = DatetimeUTC.now()Committable suggestion was skipped due to low confidence.
| def verify_market(self, market_type: MarketType, market: AgentMarket) -> bool: | ||
| if not super().verify_market(market_type, market): | ||
| return False | ||
|
|
||
| # If we have previously traded on this market, check if there is new | ||
| # relevant news that implies we should re-run a full prediction and | ||
| # potentially adjust our position. | ||
| user_id = market.get_user_id(api_keys=APIKeys()) | ||
| last_trade_datetime = market.get_most_recent_trade_datetime(user_id=user_id) | ||
| if last_trade_datetime is None: | ||
| return True | ||
|
|
||
| news = get_certified_relevant_news_since_cached( | ||
| question=market.question, | ||
| days_ago=(utcnow() - last_trade_datetime).days, | ||
| cache=self.relevant_news_response_cache, | ||
| tavily_storage=self.agent.tavily_storage, | ||
| ) | ||
| return news is not None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add logging and handle potential negative time delta.
Two suggestions for improving the market verification:
- Add logging to track decision-making
- Handle potential negative time deltas in the days_ago calculation
def verify_market(self, market_type: MarketType, market: AgentMarket) -> bool:
if not super().verify_market(market_type, market):
+ logger.debug(f"Market {market.id} failed parent verification")
return False
user_id = market.get_user_id(api_keys=APIKeys())
last_trade_datetime = market.get_most_recent_trade_datetime(user_id=user_id)
if last_trade_datetime is None:
+ logger.info(f"First trade attempt for market {market.id}")
return True
+ time_delta = max(0, (utcnow() - last_trade_datetime).days) # Ensure non-negative
news = get_certified_relevant_news_since_cached(
question=market.question,
- days_ago=(utcnow() - last_trade_datetime).days,
+ days_ago=time_delta,
cache=self.relevant_news_response_cache,
tavily_storage=self.agent.tavily_storage,
)
+ logger.info(
+ f"Market {market.id}: Found {'new' if news else 'no new'} relevant news "
+ f"in the past {time_delta} days"
+ )
return news is not None📝 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.
| def verify_market(self, market_type: MarketType, market: AgentMarket) -> bool: | |
| if not super().verify_market(market_type, market): | |
| return False | |
| # If we have previously traded on this market, check if there is new | |
| # relevant news that implies we should re-run a full prediction and | |
| # potentially adjust our position. | |
| user_id = market.get_user_id(api_keys=APIKeys()) | |
| last_trade_datetime = market.get_most_recent_trade_datetime(user_id=user_id) | |
| if last_trade_datetime is None: | |
| return True | |
| news = get_certified_relevant_news_since_cached( | |
| question=market.question, | |
| days_ago=(utcnow() - last_trade_datetime).days, | |
| cache=self.relevant_news_response_cache, | |
| tavily_storage=self.agent.tavily_storage, | |
| ) | |
| return news is not None | |
| def verify_market(self, market_type: MarketType, market: AgentMarket) -> bool: | |
| if not super().verify_market(market_type, market): | |
| logger.debug(f"Market {market.id} failed parent verification") | |
| return False | |
| # If we have previously traded on this market, check if there is new | |
| # relevant news that implies we should re-run a full prediction and | |
| # potentially adjust our position. | |
| user_id = market.get_user_id(api_keys=APIKeys()) | |
| last_trade_datetime = market.get_most_recent_trade_datetime(user_id=user_id) | |
| if last_trade_datetime is None: | |
| logger.info(f"First trade attempt for market {market.id}") | |
| return True | |
| time_delta = max(0, (utcnow() - last_trade_datetime).days) # Ensure non-negative | |
| news = get_certified_relevant_news_since_cached( | |
| question=market.question, | |
| days_ago=time_delta, | |
| cache=self.relevant_news_response_cache, | |
| tavily_storage=self.agent.tavily_storage, | |
| ) | |
| logger.info( | |
| f"Market {market.id}: Found {'new' if news else 'no new'} relevant news " | |
| f"in the past {time_delta} days" | |
| ) | |
| return news is not None |
|
CI failures unrelated |
No description provided.