Compare agent bet histories with kelly strategy #419
Conversation
WalkthroughThe changes significantly enhance the simulation of betting strategies by introducing a new data model for simulated outcomes and expanding the logic for various betting strategies. A new function computes outcomes based on market traces, and the execution flow is modified to ensure all bets have corresponding traces. Additionally, a new class for dynamically adjusting bet amounts based on market conditions is introduced, while a sanity check is removed to simplify trace retrieval. A function to retrieve private keys from Google Cloud Platform's Secret Manager is also added. Changes
Possibly related issues
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 (
|
Agent Bet vs Theoretical Kelly Bet ComparisonDeployablePredictionProphetGPT4TurboFinalAgentNumber of bets since 2024-09-13 00:00:00: 35 DeployablePredictionProphetGPT4TurboPreviewAgentNumber of bets since 2024-09-13 00:00:00: 27 DeployablePredictionProphetGPT4oAgentNumber of bets since 2024-09-13 00:00:00: 34 DeployableOlasEmbeddingOAAgentNumber of bets since 2024-09-13 00:00:00: 34 DeployableKnownOutcomeAgentNumber of bets since 2024-09-13 00:00:00: 15 |
| received_outcome_tokens = market.get_buy_token_amount( | ||
| bet_amount=market.get_bet_amount(kelly_bet.size), | ||
| direction=kelly_bet.direction, | ||
| ).amount |
There was a problem hiding this comment.
Wondering if we can get the received_outcome_tokens directly from the subgraph instead of calculating?
DId you sample a few datapoints to see if there is a diff?
There was a problem hiding this comment.
I haven't but I'm confident that it's correct based on this test:
|
I'm sorry, but I was still bugged a little. It seems that if But if we are gonna do this, then we have a problem (experiment from latest commit I pushed, seems like it doesn't show up in closed PR): |
![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 (1)
examples/monitor/match_bets_with_langfuse_traces.py (1)
124-125: Use logging instead of print statements for better practiceUsing the
loggingmodule instead ofImport the
loggingmodule and replace+import logging ... if agent_balance <= 0: - print(f"Agent died with balance {agent_balance}.") + logging.info(f"Agent died with balance {agent_balance}.") break
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- examples/monitor/match_bets_with_langfuse_traces.py (1 hunks)
- prediction_market_agent_tooling/tools/langfuse_client_utils.py (0 hunks)
Files not reviewed due to no reviewable changes (1)
- prediction_market_agent_tooling/tools/langfuse_client_utils.py
| roi = 100 * total_bet_profit / total_bet_amount | ||
| kelly_roi = 100 * total_kelly_profit / total_kelly_amount |
There was a problem hiding this comment.
Prevent potential division by zero in ROI calculations
If total_bet_amount or total_kelly_amount is zero, calculating ROI will result in a ZeroDivisionError. To prevent this, add a check before performing the division.
Update the ROI calculations to handle zero amounts safely:
roi = 100 * total_bet_profit / total_bet_amount if total_bet_amount != 0 else 0
kelly_roi = 100 * total_kelly_profit / total_kelly_amount if total_kelly_amount != 0 else 0Committable 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.
| roi = 100 * total_bet_profit / total_bet_amount | |
| kelly_roi = 100 * total_kelly_profit / total_kelly_amount | |
| roi = 100 * total_bet_profit / total_bet_amount if total_bet_amount != 0 else 0 | |
| kelly_roi = 100 * total_kelly_profit / total_kelly_amount if total_kelly_amount != 0 else 0 |
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
prediction_market_agent_tooling/tools/utils.py (1)
218-230: Add Docstring to Enhance Readability and MaintainabilityAdding a docstring to the
get_private_key_from_gcp_secretfunction will help other developers understand its purpose, parameters, and return value.Include a descriptive docstring:
def get_private_key_from_gcp_secret( secret_id: str, project_id: str, version_id: str = "latest", ) -> PrivateKey: """ Retrieves a private key from Google Cloud Secret Manager. Args: secret_id (str): The ID of the secret to retrieve. project_id (str): The Google Cloud project ID. version_id (str, optional): The version of the secret. Defaults to "latest". Returns: PrivateKey: The retrieved private key wrapped in a SecretStr. Raises: ValueError: If the private key is not found or if the secret cannot be accessed. """ # Function implementation...
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- examples/monitor/match_bets_with_langfuse_traces.py (1 hunks)
- prediction_market_agent_tooling/tools/utils.py (3 hunks)
Additional comments not posted (12)
examples/monitor/match_bets_with_langfuse_traces.py (7)
4-4: LGTM!The import statement for
BaseModelfrompydanticis correct.
9-11: LGTM!The import statement for
get_kelly_bet_fullis correct.
18-21: LGTM!The import statements for
check_not_noneandget_private_key_from_gcp_secretare correct.
24-28: LGTM!The
KellyBetOutcomedata model is defined correctly usingBaseModelfrompydantic. The fields capture the essential information about a Kelly bet outcome.
31-60: LGTM!The
get_kelly_bet_outcome_for_tracefunction is implemented correctly. It calculates the Kelly bet size, direction, correctness, and profit based on the input trace and market outcome using theget_kelly_bet_fullfunction and the market information from the trace. The function logic and implementation look good.
64-160: LGTM!The main execution block of the script is well-structured and follows a logical flow. It retrieves private keys for agents from GCP secrets, iterates over each agent, retrieves their traces and resolved bets, and compares the actual bets with theoretical Kelly bets. The simulation of the betting process using the agent's balance provides a realistic comparison between actual bets and Kelly bets. The ROI calculations for actual bets and Kelly bets provide a meaningful comparison of their performance. The code segment looks good overall.
120-124: Handle missing traces without interrupting executionRaising a
ValueErrorif some bets do not have a corresponding trace may halt the analysis unnecessarily. Since it's possible for bets to exist without traces, consider handling this scenario gracefully, perhaps by logging a warning and proceeding with the available data.Modify the code to log a warning instead of raising an exception:
if len(bets_with_traces) != len(bets): - raise ValueError( - f"{len(bets) - len(bets_with_traces)} bets do not have a corresponding trace" - ) + print( + f"Warning: {len(bets) - len(bets_with_traces)} bets do not have a corresponding trace" + )Likely invalid or redundant comment.
prediction_market_agent_tooling/tools/utils.py (5)
228-229: Avoid Exposing Sensitive Information in Error MessagesThe error message includes
secret_id, which may be sensitive. To prevent potential information leakage, avoid revealing sensitive details in exception messages.[security]
Modify the error message to exclude
secret_id:- raise ValueError(f"Private key not found in gcp secret {secret_id}") + raise ValueError("Private key not found in GCP secret")
223-230: Ensure Secure Handling of Private KeysWhen dealing with private keys, it's crucial to handle them securely to prevent unauthorized access. Ensure that the key is not logged, printed, or exposed in any way throughout the codebase.
[security]
1-1: Import ofjsonModule is AppropriateThe
jsonmodule is imported and used correctly to parse the secret payload. No issues found.
10-10: Import ofsecretmanagerModule is CorrectThe import statement
from google.cloud import secretmanageris necessary for accessing GCP Secret Manager services. No issues found.
17-17: Import ofPrivateKeyis NecessaryThe
PrivateKeyclass is imported fromprediction_market_agent_tooling.gtypesand used as the return type for the new function. No issues found.
| client = secretmanager.SecretManagerServiceClient() | ||
| name = f"projects/{project_id}/secrets/{secret_id}/versions/{version_id}" | ||
| response = client.access_secret_version(request={"name": name}) |
There was a problem hiding this comment.
Add Exception Handling for Secret Manager Operations
Accessing secrets from GCP Secret Manager can raise exceptions due to network errors, authentication issues, or missing permissions. To improve robustness, add exception handling to gracefully manage potential errors.
Wrap the secret access code in a try-except block:
try:
client = secretmanager.SecretManagerServiceClient()
name = f"projects/{project_id}/secrets/{secret_id}/versions/{version_id}"
response = client.access_secret_version(request={"name": name})
except Exception as e:
raise ValueError(f"Failed to access secret: {e}")|
|
||
| def get_private_key_from_gcp_secret( | ||
| secret_id: str, | ||
| project_id: str = "582587111398", # Gnosis AI default project_id |
There was a problem hiding this comment.
Consider Removing Hardcoded Default project_id
Hardcoding the project_id as a default parameter reduces the function's flexibility and may lead to issues in different environments or projects. It's better to require project_id as a mandatory parameter or retrieve it from an environment variable or configuration file.
Apply this diff to make project_id a required parameter:
- project_id: str = "582587111398", # Gnosis AI default project_id
+ project_id: str,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.
| project_id: str = "582587111398", # Gnosis AI default project_id | |
| project_id: str, |
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (4)
examples/monitor/match_bets_with_langfuse_traces.py (3)
Line range hint
1-34: LGTM! Consider adding docstrings for better documentation.The new imports and the
SimulatedOutcomeclass are well-structured and appropriate for the added functionality. The use of pydantic for theSimulatedOutcomeclass ensures type safety.Consider adding a docstring to the
SimulatedOutcomeclass to describe its purpose and attributes. For example:class SimulatedOutcome(BaseModel): """ Represents the outcome of a simulated bet. Attributes: size (float): The size of the bet. direction (bool): The direction of the bet (True for Yes, False for No). correct (bool): Whether the bet was correct. profit (float): The profit or loss from the bet. """ size: float direction: bool correct: bool profit: float
36-80: LGTM! Consider adding type hints for better clarity.The
get_outcome_for_tracefunction is well-implemented and handles various scenarios correctly. The assertions help ensure the expected behavior, and the profit calculation logic is sound.Consider adding type hints to the local variables for better clarity. For example:
def get_outcome_for_trace( strategy: BettingStrategy, trace: ProcessMarketTrace, market_outcome: bool, ) -> SimulatedOutcome | None: market: OmenAgentMarket = trace.market answer: ProbabilisticAnswer = trace.answer trades: list[Trade] = strategy.calculate_trades( existing_position=None, answer=ProbabilisticAnswer( p_yes=answer.p_yes, confidence=answer.confidence, ), market=market, ) # ... rest of the functionThis change would make the types of
market,answer, andtradesmore explicit, improving code readability.
83-111: LGTM! Consider parameterizing the start date for increased flexibility.The setup for the main execution block is comprehensive and well-structured. The use of GCP Secret Manager for retrieving private keys is a good security practice. The defined strategies cover a good range of betting approaches.
Consider parameterizing the start date (currently hardcoded as September 13, 2024) to make the script more flexible. You could add a command-line argument or environment variable to set this date. For example:
import argparse from datetime import datetime # At the beginning of the main block parser = argparse.ArgumentParser(description='Analyze betting strategies') parser.add_argument('--start-date', type=str, default='2024-09-13', help='Start date for analysis (YYYY-MM-DD)') args = parser.parse_args() start_time = datetime.strptime(args.start_date, '%Y-%m-%d')This change would allow users to easily specify different start dates when running the script.
prediction_market_agent_tooling/deploy/betting_strategy.py (1)
213-271: Add docstrings for the new class and its methodsIncluding docstrings for
MaxAccuracyWithKellyScaledBetsStrategyand its methods will improve code readability and help others understand the strategy's purpose and implementation.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- examples/monitor/match_bets_with_langfuse_traces.py (2 hunks)
- prediction_market_agent_tooling/deploy/betting_strategy.py (2 hunks)
Additional comments not posted (2)
prediction_market_agent_tooling/deploy/betting_strategy.py (2)
144-146: Good addition of__repr__method for better debuggingThe
__repr__method provides a useful string representation of the object, enhancing debugging and logging capabilities.
209-211:__repr__method improves object representationAdding the
__repr__method makes it easier to understand instances when printed or logged.
| print("# Agent Bet vs Simulated Bet Comparison") | ||
| for agent_name, private_key in agent_pkey_map.items(): | ||
| print(f"\n## {agent_name}\n") | ||
| api_keys = APIKeys(BET_FROM_PRIVATE_KEY=private_key) | ||
|
|
||
| # Pick a time after pool token number is stored in OmenAgentMarket | ||
| start_time = datetime(2024, 9, 13) | ||
|
|
||
| langfuse = Langfuse( | ||
| secret_key=api_keys.langfuse_secret_key.get_secret_value(), | ||
| public_key=api_keys.langfuse_public_key, | ||
| host=api_keys.langfuse_host, | ||
| ) | ||
|
|
||
| traces = get_traces_for_agent( | ||
| agent_name=agent_name, | ||
| trace_name="process_market", | ||
| from_timestamp=start_time, | ||
| has_output=True, | ||
| client=langfuse, | ||
| ) | ||
| process_market_traces: list[ProcessMarketTrace] = [] | ||
| for trace in traces: | ||
| if process_market_trace := ProcessMarketTrace.from_langfuse_trace(trace): | ||
| process_market_traces.append(process_market_trace) | ||
|
|
||
| bets: list[ResolvedBet] = OmenAgentMarket.get_resolved_bets_made_since( | ||
| better_address=api_keys.bet_from_address, | ||
| start_time=start_time, | ||
| end_time=None, | ||
| ) | ||
|
|
||
| # All bets should have a trace, but not all traces should have a bet | ||
| # (e.g. if all markets are deemed unpredictable), so iterate over bets | ||
| bets_with_traces: list[ResolvedBetWithTrace] = [] | ||
| for bet in bets: | ||
| trace = get_trace_for_bet(bet, process_market_traces) | ||
| if trace: | ||
| bets_with_traces.append(ResolvedBetWithTrace(bet=bet, trace=trace)) | ||
|
|
||
| print(f"Number of bets since {start_time}: {len(bets_with_traces)}\n") | ||
| if len(bets_with_traces) != len(bets): | ||
| raise ValueError( | ||
| f"{len(bets) - len(bets_with_traces)} bets do not have a corresponding trace" | ||
| ) | ||
|
|
There was a problem hiding this comment.
LGTM! Consider improving error handling for mismatched bets and traces.
The data retrieval and processing logic is well-implemented. Matching bets with corresponding traces and checking for data integrity are crucial steps.
The current error handling for mismatched bets and traces could be improved. Instead of raising a ValueError, consider logging a warning and continuing with the available data. This approach would allow the script to process as much data as possible, even if some bets are missing traces.
Replace the following code:
if len(bets_with_traces) != len(bets):
raise ValueError(
f"{len(bets) - len(bets_with_traces)} bets do not have a corresponding trace"
)with:
import logging
# ... (earlier in the file, set up logging)
if len(bets_with_traces) != len(bets):
missing_traces = len(bets) - len(bets_with_traces)
logging.warning(f"{missing_traces} bets do not have a corresponding trace. Continuing with available data.")
print(f"Warning: {missing_traces} bets do not have a corresponding trace. Continuing with available data.")This change will allow the script to continue processing even if some bets are missing traces, providing more robust behavior.
| simulations: list[dict[str, Any]] = [] | ||
|
|
||
| for strategy_idx, strategy in enumerate(strategies): | ||
| # "Born" agent with initial funding, simulate as if he was doing bets one by one. | ||
| starting_balance = 50.0 | ||
| agent_balance = starting_balance | ||
| simulated_outcomes: list[SimulatedOutcome] = [] | ||
|
|
||
| for bet_with_trace in bets_with_traces: | ||
| bet = bet_with_trace.bet | ||
| trace = bet_with_trace.trace | ||
| simulated_outcome = get_outcome_for_trace( | ||
| strategy=strategy, trace=trace, market_outcome=bet.market_outcome | ||
| ) | ||
| if simulated_outcome is None: | ||
| continue | ||
| simulated_outcomes.append(simulated_outcome) | ||
| agent_balance += simulated_outcome.profit | ||
|
|
||
| total_bet_amount = sum([bt.bet.amount.amount for bt in bets_with_traces]) | ||
| total_bet_profit = sum([bt.bet.profit.amount for bt in bets_with_traces]) | ||
| total_simulated_amount = sum([so.size for so in simulated_outcomes]) | ||
| total_simulated_profit = sum([so.profit for so in simulated_outcomes]) | ||
| roi = 100 * total_bet_profit / total_bet_amount | ||
| simulated_roi = 100 * total_simulated_profit / total_simulated_amount | ||
|
|
||
| # At the beginning, add also the agent's current strategy. | ||
| if strategy_idx == 0: | ||
| simulations.append( | ||
| { | ||
| "strategy": "original", | ||
| "bet_amount": total_bet_amount, | ||
| "bet_profit": total_bet_profit, | ||
| "roi": roi, | ||
| # We don't know these for the original run. | ||
| "start_balance": None, | ||
| "end_balance": None, | ||
| } | ||
| ) | ||
|
|
||
| simulations.append( | ||
| { | ||
| "strategy": repr(strategy), | ||
| "bet_amount": total_simulated_amount, | ||
| "bet_profit": total_simulated_profit, | ||
| "roi": simulated_roi, | ||
| "start_balance": starting_balance, | ||
| "end_balance": agent_balance, | ||
| } | ||
| ) | ||
|
|
||
| print(pd.DataFrame.from_records(simulations).to_markdown(index=False)) |
There was a problem hiding this comment.
LGTM! Consider adding summary statistics for easier comparison.
The simulation logic and results presentation are well-implemented. The comparison of ROI between actual bets and various simulated strategies provides valuable insights.
To enhance the analysis of results, consider adding summary statistics to compare the performance of different strategies more easily. You could add the following code after the DataFrame is created:
df = pd.DataFrame.from_records(simulations)
print(df.to_markdown(index=False))
# Add summary statistics
print("\nSummary Statistics:")
summary = df.groupby('strategy').agg({
'roi': ['mean', 'std', 'min', 'max'],
'bet_amount': 'sum',
'bet_profit': 'sum'
}).reset_index()
summary.columns = ['strategy', 'mean_roi', 'std_roi', 'min_roi', 'max_roi', 'total_bet_amount', 'total_profit']
print(summary.to_markdown(index=False))This addition will provide a concise overview of each strategy's performance, making it easier to compare and identify the most effective approaches.
| def adjust_bet_amount( | ||
| self, existing_position: Position | None, market: AgentMarket | ||
| ) -> float: | ||
| existing_position_total_amount = ( | ||
| existing_position.total_amount.amount if existing_position else 0 | ||
| ) | ||
| return self.max_bet_amount + existing_position_total_amount | ||
|
|
There was a problem hiding this comment.
Ensure max_bet_amount enforces the intended maximum
In the adjust_bet_amount method, adding existing_position_total_amount to self.max_bet_amount might result in a total bet exceeding max_bet_amount when there's an existing position. If max_bet_amount is intended to cap the total bet amount, consider revising the calculation to enforce this limit.
| estimated_p_yes = float(answer.p_yes > 0.5) | ||
| confidence = 1.0 | ||
|
|
||
| kelly_bet = ( |
There was a problem hiding this comment.
Consider using actual estimated probabilities and confidence
Currently, estimated_p_yes is set to 1.0 or 0.0 based on whether answer.p_yes > 0.5, and confidence is hardcoded to 1.0. This may not fully utilize the model's probability estimates and confidence levels. To leverage the Kelly criterion effectively, consider using the actual answer.p_yes and answer.confidence values.
| adjusted_bet_amount = self.adjust_bet_amount(existing_position, market) | ||
| outcome_token_pool = check_not_none(market.outcome_token_pool) | ||
|
|
||
| # Fixed direction of bet, only use Kelly to adjust the bet size based on market's outcome pool size. | ||
| estimated_p_yes = float(answer.p_yes > 0.5) | ||
| confidence = 1.0 | ||
|
|
||
| kelly_bet = ( | ||
| get_kelly_bet_full( | ||
| yes_outcome_pool_size=outcome_token_pool[ | ||
| market.get_outcome_str_from_bool(True) | ||
| ], | ||
| no_outcome_pool_size=outcome_token_pool[ | ||
| market.get_outcome_str_from_bool(False) | ||
| ], | ||
| estimated_p_yes=estimated_p_yes, | ||
| max_bet=adjusted_bet_amount, | ||
| confidence=confidence, | ||
| ) | ||
| if market.has_token_pool() | ||
| else get_kelly_bet_simplified( | ||
| adjusted_bet_amount, | ||
| market.current_p_yes, | ||
| estimated_p_yes, | ||
| confidence, | ||
| ) | ||
| ) | ||
|
|
||
| amounts = { | ||
| market.get_outcome_str_from_bool(kelly_bet.direction): TokenAmount( | ||
| amount=kelly_bet.size, currency=market.currency | ||
| ), | ||
| } | ||
| target_position = Position(market_id=market.id, amounts=amounts) | ||
| trades = self._build_rebalance_trades_from_positions( | ||
| existing_position, target_position, market=market | ||
| ) | ||
| return trades |
There was a problem hiding this comment.
Refactor shared logic into a common method
The calculate_trades method contains logic similar to that in KellyBettingStrategy. To promote code reusability and reduce duplication, consider refactoring the common code for calculating the Kelly bet into a shared method or utility function.
See output in the comments when running the modified examples/monitor/match_bets_with_langfuse_traces.py script.
TL;DR Kelly gives consistently better results! 🚀