Refactor DeployableTraderAgent to 1. predict a probability for a market then 2. place a bet based on its BettingStrategy
#352
Conversation
WalkthroughThe recent updates across the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DeployableTraderAgent
participant BettingStrategy
participant Market
User->>DeployableTraderAgent: Place Bet
DeployableTraderAgent->>BettingStrategy: Calculate Bet Amount and Direction
BettingStrategy->>Market: Validate Market Conditions
Market->>BettingStrategy: Return Market Status
BettingStrategy-->>DeployableTraderAgent: Return Calculated Bet
DeployableTraderAgent-->>User: Confirm Bet Placement
Possibly related issues
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
…ge-default-betting-strategy-from-amount-==-tiny_bet-direction-==-yes-if-p_yes-05-to-use-kelly-bet # Conflicts: # poetry.lock
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
prediction_market_agent_tooling/deploy/betting_strategy.py (1)
1-61: New betting strategies file approved.The new file introduces
FixedBetBettingStrategyandKellyBettingStrategy. The use of abstract classes and methods is appropriate and enhances modularity.Consider adding logging for the calculated bet amounts and directions to aid in debugging and analysis.
+ import logging + logger = logging.getLogger(__name__) class FixedBetBettingStrategy(BettingStrategy, metaclass=ABCMeta): ... def calculate_bet_amount_and_direction( self, answer: Answer, market: AgentMarket ) -> TokenAmountAndDirection: bet_amount = self.get_bet_amount_for_market(market) + logger.debug(f"FixedBet: Market: {market}, Bet Amount: {bet_amount}, Direction: {answer.decision}") return TokenAmountAndDirection( amount=bet_amount, currency=market.currency, direction=answer.decision, ) class KellyBettingStrategy(BettingStrategy, metaclass=ABCMeta): ... def calculate_bet_amount_and_direction( self, answer: Answer, market: AgentMarket ) -> TokenAmountAndDirection: max_bet_amount = self.get_max_bet_amount_for_market() kelly_bet = get_kelly_bet( max_bet_amount, market.current_p_yes, answer.p_yes, answer.confidence ) + logger.debug(f"KellyBet: Market: {market}, Bet Size: {kelly_bet.size}, Direction: {kelly_bet.direction}") return TokenAmountAndDirection( amount=kelly_bet.size, currency=market.currency, direction=kelly_bet.direction, )prediction_market_agent_tooling/markets/polymarket/polymarket.py (1)
Line range hint
1-1: Removal ofget_tiny_bet_amountapproved.The removal of the unimplemented
get_tiny_bet_amountmethod simplifies the class interface. This change clarifies the current capabilities ofPolymarketAgentMarket.Consider planning the implementation of the
place_betmethod to complete the class functionality.Would you like assistance in drafting an implementation plan for the
place_betmethod?tests/markets/test_betting_strategies.py (1)
206-206: Simplify the boolean expression.Use
not ...instead ofFalse if ... else Truefor clarity.Apply the following change:
- expected_bet_direction = False if est_p_yes < market_p_yes else True + expected_bet_direction = not est_p_yes < market_p_yesTools
Ruff
206-206: Use
not ...instead ofFalse if ... else TrueReplace with
not ...(SIM211)
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 (13)
- prediction_market_agent_tooling/benchmark/utils.py (1 hunks)
- prediction_market_agent_tooling/deploy/agent.py (7 hunks)
- prediction_market_agent_tooling/deploy/agent_example.py (1 hunks)
- prediction_market_agent_tooling/deploy/betting_strategy.py (1 hunks)
- prediction_market_agent_tooling/markets/agent_market.py (1 hunks)
- prediction_market_agent_tooling/markets/data_models.py (2 hunks)
- prediction_market_agent_tooling/markets/manifold/manifold.py (1 hunks)
- prediction_market_agent_tooling/markets/omen/omen.py (1 hunks)
- prediction_market_agent_tooling/markets/polymarket/polymarket.py (1 hunks)
- prediction_market_agent_tooling/tools/betting_strategies/kelly_criterion.py (4 hunks)
- scripts/sell_all_omen.py (2 hunks)
- tests/markets/test_betting_strategies.py (2 hunks)
- tests/test_answer.py (1 hunks)
Files skipped from review due to trivial changes (2)
- prediction_market_agent_tooling/benchmark/utils.py
- prediction_market_agent_tooling/markets/omen/omen.py
Additional context used
Ruff
tests/markets/test_betting_strategies.py
206-206: Use
not ...instead ofFalse if ... else TrueReplace with
not ...(SIM211)
Additional comments not posted (12)
tests/test_answer.py (1)
5-5: Import update approved.The import path for the
Answerclass has been updated to reflect its new location. This change is consistent with the restructuring of the codebase.prediction_market_agent_tooling/deploy/agent_example.py (1)
5-5: Import update approved.The import path for the
Probabilitytype has been updated to reflect its new location. This change is consistent with the restructuring of the codebase.prediction_market_agent_tooling/tools/betting_strategies/kelly_criterion.py (1)
1-1: Refactor approved with verification suggestion.The function
get_kelly_betnow returns aKellyResultinstead of aKellyBet, and the logic for determining bet direction has been simplified. These changes enhance clarity and efficiency.Ensure that all usages of the
get_kelly_betfunction are updated to handle the newKellyResultreturn type.Run the following script to verify the function usage:
Also applies to: 14-55
Verification successful
All usages of
get_kelly_betcorrectly handle theKellyResultreturn type. The function is used in both test and deployment files, where its return value is accessed appropriately. No further changes are needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `get_kelly_bet` handle the new return type. # Test: Search for the function usage. Expect: Only occurrences of handling `KellyResult`. rg --type python -A 5 $'get_kelly_bet'Length of output: 68
Script:
#!/bin/bash # Description: Verify all function calls to `get_kelly_bet` handle the new return type. # Test: Search for the function usage. Expect: Only occurrences of handling `KellyResult`. rg 'get_kelly_bet' --glob '*.py' -A 5Length of output: 2572
scripts/sell_all_omen.py (1)
65-67: LGTM! Verify the new logic for skipping bets.The introduction of
minimum_token_amount_for_sellingenhances readability and maintainability.Ensure that the logic for skipping bets based on this new variable is correctly integrated and doesn't introduce any unintended behavior.
Run the following script to verify the logic:
Verification successful
Verification Successful: Logic for Skipping Bets is Correctly Integrated
The logic for skipping bets using
minimum_token_amount_for_sellingis correctly implemented. The condition checks ifcurrent_token_balance.amountis less than or equal tominimum_token_amount_for_selling.amount, which ensures that bets are skipped appropriately.
- File:
scripts/sell_all_omen.py
- Line: Conditional check using
minimum_token_amount_for_selling.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the logic for skipping bets based on the new variable. # Test: Search for the usage of `minimum_token_amount_for_selling`. Expect: Consistent logic for skipping bets. rg --type python 'minimum_token_amount_for_selling'Length of output: 82
Script:
#!/bin/bash # Description: Verify the logic for skipping bets based on the new variable. # Test: Search for the usage of `minimum_token_amount_for_selling`. Expect: Consistent logic for skipping bets. rg --glob '*.py' 'minimum_token_amount_for_selling'Length of output: 239
prediction_market_agent_tooling/markets/data_models.py (5)
60-62: LGTM! New classKellyResultis well-defined.The
KellyResultclass encapsulates direction and size attributes effectively.
65-66: LGTM! New classTokenAmountAndDirectionis well-defined.The
TokenAmountAndDirectionclass extendsTokenAmountwith a direction attribute, adding useful context.
89-89: LGTM! TheDecisiontype is well-implemented.The use of
AnnotatedwithBeforeValidatorensures that decisions adhere to the expected boolean format.
92-96: LGTM! TheAnswerclass update is appropriate.The inclusion of the
Decisiontype in theAnswerclass emphasizes validated decision-making.
69-86: LGTM! Verify the usage ofto_boolean_outcome.The
to_boolean_outcomefunction is robust and handles various input types effectively.Ensure that this function is used consistently across the codebase to maintain data integrity.
Run the following script to verify the function usage:
prediction_market_agent_tooling/deploy/agent.py (3)
9-12: Import changes look good.The imports for
BettingStrategyandFixedBetBettingStrategyare necessary for the new betting strategy functionality.
186-186: Strategy attribute addition is appropriate.The addition of the
strategyattribute initialized withFixedBetBettingStrategyenhances the flexibility of the betting logic.
279-289: Methodcalculate_bet_amount_and_directionis well-implemented.The method effectively integrates the strategy into the betting process and includes necessary validation for currency consistency.
Ensure that the
calculate_bet_amount_and_directionmethod in the strategy is implemented correctly and handles all edge cases.Run the following script to verify the method implementation in the strategy:
Verification successful
Multiple Implementations of
calculate_bet_amount_and_directionFoundThe
calculate_bet_amount_and_directionmethod is implemented multiple times inbetting_strategy.py, each with different logic. Ensure that these implementations handle all edge cases and integrate correctly with the agent's method inagent.py.
- File:
prediction_market_agent_tooling/deploy/betting_strategy.py
- Lines: Multiple implementations found; verify logic and edge case handling.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `calculate_bet_amount_and_direction` in the strategy. # Test: Search for the method implementation. Expect: Correct handling of edge cases. ast-grep --lang python --pattern $'class $_ { $$$ def calculate_bet_amount_and_direction($_, $_) { $$$ } $$$ }'Length of output: 471
Script:
#!/bin/bash # Description: Refine the search for the `calculate_bet_amount_and_direction` method in any class. # Corrected pattern to search for the method implementation. ast-grep --lang python --pattern $'class $_ { $$$ def calculate_bet_amount_and_direction(self, answer, market): $$$ } $$$ }'Length of output: 664
Script:
#!/bin/bash # Description: Simplify the search pattern to locate the `calculate_bet_amount_and_direction` method in any class. # Simplified pattern to search for the method implementation. ast-grep --lang python --pattern $'def calculate_bet_amount_and_direction(self, $_, $_):'Length of output: 90
Script:
#!/bin/bash # Description: Search for any method containing "bet_amount" to find related implementations. # Use ripgrep to find methods with "bet_amount" in their name. rg --pcre2 --glob '*.py' -A 5 'def .*bet_amount.*\('Length of output: 3949
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- prediction_market_agent_tooling/tools/gnosis_rpc.py (1 hunks)
Additional comments not posted (1)
prediction_market_agent_tooling/tools/gnosis_rpc.py (1)
6-6: Verify the impact of changing the defaultGNOSIS_RPC_URL.The default RPC URL for the Gnosis network has been updated. Ensure that this change aligns with the intended functionality and that any dependencies or documentation are updated accordingly.
Run the following script to verify the usage of
GNOSIS_RPC_URLacross the codebase and ensure that the change is consistent with the system's requirements:
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- prediction_market_agent_tooling/tools/web3_utils.py (1 hunks)
Additional context used
Learnings (1)
Common learnings
Learnt from: kongzii PR: gnosis/prediction-market-agent-tooling#224 File: prediction_market_agent_tooling/markets/markets.py:67-67 Timestamp: 2024-04-29T14:14:38.751Z Learning: The function `have_bet_on_market_since` is suggested to be refactored into a method on the `AgentMarket` class, potentially reusing code from `DeployedAgent.get_resolved_bets`. This task is planned for future work, not within the scope of the current PR.
Additional comments not posted (1)
prediction_market_agent_tooling/tools/web3_utils.py (1)
160-160: LGTM! Verify the retry logic for new error condition.The addition of the "ALREADY_EXISTS" error to the retry condition is a robust enhancement. Ensure that this error condition is correctly handled in all relevant scenarios.
To verify the retry logic, consider running tests that simulate the "ALREADY_EXISTS" error condition.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
ape-config.yamlis excluded by!**/*.yamlpoetry.lockis excluded by!**/*.lock,!**/*.lockpyproject.tomlis excluded by!**/*.toml
Files selected for processing (1)
- prediction_market_agent_tooling/tools/web3_utils.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- prediction_market_agent_tooling/tools/web3_utils.py
Additional context used
Learnings (1)
Common learnings
Learnt from: kongzii PR: gnosis/prediction-market-agent-tooling#224 File: prediction_market_agent_tooling/markets/markets.py:67-67 Timestamp: 2024-04-29T14:14:38.751Z Learning: The function `have_bet_on_market_since` is suggested to be refactored into a method on the `AgentMarket` class, potentially reusing code from `DeployedAgent.get_resolved_bets`. This task is planned for future work, not within the scope of the current PR.
|
Closes #309 |
scripts/sell_all_omen.py
Outdated
| @@ -61,7 +62,10 @@ def sell_all( | |||
| outcome = agent_market.outcomes[bet.outcomeIndex] | |||
| current_token_balance = agent_market.get_token_balance(better_address, outcome) | |||
|
|
|||
| if current_token_balance.amount <= OmenAgentMarket.get_tiny_bet_amount().amount: | |||
| minimum_token_amount_for_selling = BetAmount( | |||
| amount=0.00001, currency=OmenAgentMarket.currency | |||
There was a problem hiding this comment.
Maybe we can keep the .get_tiny_bet_amount so that 0.0001 is defined on one place instead of two?
There was a problem hiding this comment.
abstracted to a common variable and refactored both usages
There was a problem hiding this comment.
But why not just keep get_tiny_bet_amount as it is and use that? What was the motivation to remove it? (is there a need for that? from the code it doesn't look like it)
We have AgentMarket as a abstract class that our agents can work with, if something is subclass of AgentMarket, agent should just work with it (if all methods are implemented correctly).
But now, if we introduce a new SomeNewMarket(AgentMarket), it's not clear that there are places in the code where we accept AgentMarket, yet it won't work.
There was a problem hiding this comment.
It was an implementation decision. Certainly get_tiny_bet_amount used to work, but in your example, if we have a new market (Polymarket(AgentMarket), we would have to overwrite get_tiny_bet_amount implementation for that subclass - same here, we extend the logic on the FixedBettingStrategy. This is always needed because the bet_amount involves currency, which is different from market type to market type.
But now, if we introduce a new SomeNewMarket(AgentMarket), it's not clear that there are places in the code where we accept AgentMarket, yet it won't work.
The same will happen, only the exception will be different -NotImplementedErrorvsValueError(from the Strategyget_max_bet_amount_for_market).
There was a problem hiding this comment.
This is always needed
It was needed, but the difference is that it was forced. Because AgentMartet class has many such methods:
So, if we add new market support, it should be as hard as possible to forget about something.
That was the idea behind AgentMarket. If we go with this other way of doing things, we will need to keep track of all the places around the code that need an update to make them work with basic agent stuff. And then AgentMarket class could be removed.
There was a problem hiding this comment.
@kongzii, I've created the issue as requested. You can track it here: Issue #358. This will help ensure that the AgentMarket class is refactored to use abstract methods, enhancing code robustness and preventing runtime errors. Let me know if there's anything else you'd like to address!
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a GitHub issue to refactor the `AgentMarket` class to use abstract methods.
gh issue create --title "Refactor AgentMarket class to use abstract methods" --body "Refactor the \`AgentMarket\` class to make it an abstract class and use the \`@abstractmethod\` decorator for methods currently raising \`NotImplementedError\`. This change will enforce the implementation of necessary methods in subclasses, providing compile-time warnings if they are not implemented. This approach will enhance code robustness and prevent runtime errors. See the discussion for more details: https://github.com/gnosis/prediction-market-agent-tooling/pull/352#discussion_r1724570075" --assignee kongzii --repo gnosis/prediction-market-agent-tooling
Length of output: 715
Learnings used
Learnt from: kongzii
PR: gnosis/prediction-market-agent-tooling#224
File: prediction_market_agent_tooling/markets/markets.py:67-67
Timestamp: 2024-04-29T14:14:38.751Z
Learning: The function `have_bet_on_market_since` is suggested to be refactored into a method on the `AgentMarket` class, potentially reusing code from `DeployedAgent.get_resolved_bets`. This task is planned for future work, not within the scope of the current PR.
There was a problem hiding this comment.
ABC pattern is ofc great and new issue makes sense.
Still, I don't see a way where we keep tiny_bet_amount and use kelly, except simply hijacking the method get_tiny_bet_amount with some bool_flag (e.g.use_kelly), which then implements a new logic.
Maybe you have a better suggestion?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Yeah, it seems like a misunderstanding.
KellyBettingStrategy is great; no change is needed there from my point of view.
FixedBetBettingStrategy could be renamed to TinyBetStrategy and use the get_tiny_bet_amount method of the market class. It makes sense to me that TinyBetStrategy would be the default strategy, because it's safe, and it will just work with arbitrary AgentMarket.
FixedBetBettingStrategy can exist as well, but it would take a amount argument in its __init__ method, and again, it doesn't have to use any get_tiny_bet_amount-like implementation, just return the fixed value.
wdyt?
There was a problem hiding this comment.
I implemented a mix, see 683b95f#diff-ebe828b1c77960ecaea62858ada5f1ee0b662b19051aa2ffbf92cd6ea6287110R25
I like the idea of using get_tiny_bet_amount, because this will break (mypy will complain) if another AgentMarket is implemented which does not have the method get_tiny_bet_amount. Together with ABC from the other issue, this is robust enough for me.
| @@ -316,12 +302,17 @@ def process_bets(self, market_type: MarketType) -> None: | |||
| logger.info(f"Skipping market {market} as no answer was provided") | |||
| continue | |||
| if self.place_bet: | |||
| amount = self.calculate_bet_amount(result, market) | |||
| amount_and_direction = self.calculate_bet_amount_and_direction( | |||
There was a problem hiding this comment.
It feels to me odd that we're computing an Answer above with result = self.answer_binary_market(market) (which already contains a bet direction == result.decision) and then computing a direction again here.
Not sure how much of a can of worms that is to refactor, but I think that should come first (or as part of this PR)
There was a problem hiding this comment.
That's an important aspect.
It feels to me odd that we're computing an Answer above with result = self.answer_binary_market(market) (which already contains a bet direction == result.decision) and then computing a direction again here.
We need the Answer because it contains estimated_p_yes, required by the Kelly criterion (which can then revert the direction). To compare that with an agent case, the TTA creates an Answer object with a "black-box" logic - if we provide the Kelly strategy, then that's how we want to calculate bet sizes | direction.
Another idea I see is having the agents simply output p_yes, rationale, etc but NOT directions. In this case it's clear how direction was defined.
Problem with that is that FixedBettingStrategy copies direction from Answer - so in this case, we would need a logic for deciding direction (e.g. p_yes > 0.5, go YES) or similar, but also observing the market p_yes.
There was a problem hiding this comment.
Another idea I see is having the agents simply output p_yes, rationale, etc but NOT directions.
Yes, this one!! This is the refactor I meant in the description for the ticket (#338 (comment)) 😁 (sorry if it wasn't very clear).
There was a problem hiding this comment.
Makes sense. Removed direction from Answer and adapted the method, will change this in PMA when agents produce Answers.
There was a problem hiding this comment.
Implemented this, see ProbabilisticAnswer - also adapted PMA to restrain the agent from producing Direction - this will be done by the Strategy itself using estimate_p_yes and market.current_p_yes
Co-authored-by: Peter Jung <peter@jung.ninja>
Co-authored-by: Peter Jung <peter@jung.ninja>
Co-authored-by: Peter Jung <peter@jung.ninja>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- prediction_market_agent_tooling/deploy/betting_strategy.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- prediction_market_agent_tooling/deploy/betting_strategy.py
prediction_market_agent_tooling/tools/betting_strategies/kelly_criterion.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Evan Griffiths <56087052+evangriffiths@users.noreply.github.com>
Co-authored-by: Evan Griffiths <56087052+evangriffiths@users.noreply.github.com>
…egy-from-amount-==-tiny_bet-direction-==-yes-if-p_yes-05-to-use-kelly-bet' into 338-change-default-betting-strategy-from-amount-==-tiny_bet-direction-==-yes-if-p_yes-05-to-use-kelly-bet
Co-authored-by: Evan Griffiths <56087052+evangriffiths@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (9)
prediction_market_agent_tooling/deploy/agent_example.py (2)
16-18: Consider improving the randomness explanation.The reasoning for the decision in
DeployableCoinFlipAgentcould be more descriptive to clarify the randomness.Consider updating the
reasoningattribute to explain the randomness more explicitly, such as "Randomly decided based on a coin flip."
26-27: Clarify the exception message.The
RuntimeErrormessage inDeployableAlwaysRaiseAgentcould be more informative.Consider updating the message to something like "This agent is configured to always raise an exception."
prediction_market_agent_tooling/deploy/betting_strategy.py (4)
13-18: Ensure proper documentation for the abstract class.The
BettingStrategyclass serves as an abstract base class for betting strategies. Consider adding a class-level docstring to describe its purpose and usage.class BettingStrategy(ABC): + """ + Abstract base class for defining betting strategies. + """
21-27: Consider renamingcalculate_directionfor clarity.The method name
calculate_directionmight be more descriptive if it reflects its purpose, such asdetermine_bet_direction.def calculate_direction(market_p_yes: float, estimate_p_yes: float) -> bool: - # If estimate_p_yes >= market.current_p_yes, then bet TRUE, else bet FALSE. + # If estimate_p_yes >= market_p_yes, then bet TRUE, else bet FALSE.
29-38: Ensure consistency in method documentation.The
calculate_bet_amount_and_directionmethod lacks a docstring. Adding one would improve readability and maintainability.def calculate_bet_amount_and_direction( + """ + Calculate the bet amount and direction based on the market and answer probabilities. + """
47-58: Ensure consistency in method documentation.The
calculate_bet_amount_and_directionmethod lacks a docstring. Adding one would improve readability and maintainability.def calculate_bet_amount_and_direction( + """ + Calculate the bet amount and direction using the Kelly criterion. + """tests/test_benchmark.py (3)
Line range hint
48-54: Ensure comprehensive test coverage.The
test_agent_predictionfunction correctly tests the updated prediction logic. Consider adding edge cases to ensure robustness.def test_agent_prediction(dummy_agent: DummyAgent) -> None: # Existing assertions... # Add edge case tests here
Line range hint
64-73: Ensure cache functionality is robust.The
test_cachefunction tests the cache save and load functionality. Ensure that it handles edge cases, such as corrupted files or missing data.def test_cache() -> None: # Existing test logic... # Add tests for edge cases
Line range hint
76-111: Ensure benchmarker cache functionality is robust.The
test_benchmarker_cachefunction tests the benchmarker cache functionality. Ensure that it handles edge cases, such as cache misses or invalid data.def test_benchmarker_cache(dummy_agent: DummyAgent) -> None: # Existing test logic... # Add tests for edge cases
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 (7)
- prediction_market_agent_tooling/benchmark/agents.py (2 hunks)
- prediction_market_agent_tooling/deploy/agent.py (8 hunks)
- prediction_market_agent_tooling/deploy/agent_example.py (2 hunks)
- prediction_market_agent_tooling/deploy/betting_strategy.py (1 hunks)
- prediction_market_agent_tooling/tools/betting_strategies/kelly_criterion.py (2 hunks)
- tests/test_benchmark.py (3 hunks)
- tests/test_betting_strategy.py (1 hunks)
Additional comments not posted (13)
tests/test_betting_strategy.py (1)
1-20: Good start with the test cases! Consider adding edge cases.The test cases for
MaxAccuracyBettingStrategyare well-structured and cover basic scenarios.Consider adding edge cases, such as when
estimate_p_yesequalsmarket_p_yes, to ensure comprehensive test coverage.prediction_market_agent_tooling/deploy/agent_example.py (1)
5-5: Approved: Transition toProbabilisticAnswer.The change to use
ProbabilisticAnswerenhances the probabilistic modeling approach.Ensure that all dependent code is updated to handle
ProbabilisticAnswer.prediction_market_agent_tooling/tools/betting_strategies/kelly_criterion.py (3)
4-6: Approved: Probability validation function.The
check_is_valid_probabilityfunction effectively ensures that probabilities are within a valid range.Consider adding a docstring to describe its purpose.
10-10: Approved: Simplification ofdirectionattribute.The change to use a boolean for
directioninKellyBetsimplifies the representation.Ensure that all dependent code is updated to handle the boolean
direction.
42-46: Approved: Kelly Criterion implementation.The
get_kelly_betfunction correctly implements the Kelly Criterion for calculating bet sizes.Consider adding comments to explain the logic behind choosing the
bet_directionand handlingmarket_prob.prediction_market_agent_tooling/benchmark/agents.py (1)
Line range hint
50-57: Simplification of prediction logic inFixedAgent.The
predictmethod now directly assignsp_yesbased onfixed_answer. This change simplifies the logic and aligns with the agent's purpose.tests/test_benchmark.py (3)
Line range hint
57-61: Verify benchmark run logic.The
test_benchmark_runfunction appears to correctly test the benchmark logic. Ensure that it covers all relevant scenarios, especially with the updated prediction logic.
Line range hint
114-128: Verify handling of cancelled markets.The
test_benchmarker_cancelled_marketsfunction tests the handling of cancelled markets. Ensure that it covers all relevant scenarios and correctly raises exceptions.
Line range hint
131-186: Verify probable resolution logic.The
test_market_probable_resolutionfunction tests the probable resolution logic. Ensure that it covers all relevant scenarios and correctly handles unexpected resolutions.prediction_market_agent_tooling/deploy/agent.py (4)
281-281: LGTM!The initialization of the
strategyattribute withMaxAccuracyBettingStrategyis consistent with the PR's objectives.
314-324: LGTM!The
calculate_bet_amount_and_directionmethod is well-structured and includes appropriate error handling for currency mismatches.
375-377: LGTM!The change in return type to
ProbabilisticAnswer | Nonealigns with the new strategy implementation.
425-443: LGTM!The use of
calculate_bet_amount_and_directionand the added logging enhance the clarity and robustness of the betting process.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- prediction_market_agent_tooling/deploy/betting_strategy.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- prediction_market_agent_tooling/deploy/betting_strategy.py
| @@ -47,10 +40,10 @@ def get_kelly_bet( | |||
| check_is_valid_probability(confidence) | |||
|
|
|||
| if estimated_p_yes > market_p_yes: | |||
| bet_direction = BetDirection.YES | |||
| bet_direction = True | |||
There was a problem hiding this comment.
Why was this change from a YES enum to a boolean required?
There was a problem hiding this comment.
This was discussed in another thread (can't remember which one from the 80 comments) - if you have specific concerns, happy to open a new PR to discuss having "YES" or True.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock,!**/*.lock
Files selected for processing (2)
- prediction_market_agent_tooling/deploy/agent.py (8 hunks)
- prediction_market_agent_tooling/deploy/betting_strategy.py (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- prediction_market_agent_tooling/deploy/agent.py
- prediction_market_agent_tooling/deploy/betting_strategy.py
gabrielfior
deleted the
338-change-default-betting-strategy-from-amount-==-tiny_bet-direction-==-yes-if-p_yes-05-to-use-kelly-bet
branch
evangriffiths
changed the title
BettingStrategy
No description provided.