-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor microagent MarketFunctions to take market_id as input #66
Conversation
WalkthroughThe updates primarily focus on enhancing type safety, renaming functions for clarity, and adjusting market-related functionalities across various modules. Changes include modifying type hints, updating function names and logic, and refining market handling and testing within the prediction market agent tooling. Changes
Possibly related issues
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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 Configration File (
|
@@ -39,7 +39,7 @@ def to_market(self) -> AgentMarket: | |||
), | |||
volume=None, | |||
created_time=None, | |||
close_time=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Required change after bumping PMAT version
@@ -33,7 +34,7 @@ class DeployableKnownOutcomeAgent(DeployableAgent): | |||
def load(self) -> None: | |||
self.markets_with_known_outcomes: dict[str, Result] = {} | |||
|
|||
def pick_markets(self, markets: list[AgentMarket]) -> list[AgentMarket]: | |||
def pick_markets(self, markets: t.Sequence[AgentMarket]) -> list[AgentMarket]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Required change after bumping PMAT version
@@ -63,11 +62,18 @@ def __init__(self, market_type: MarketType) -> None: | |||
self.market_type = market_type | |||
super().__init__() | |||
|
|||
@property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added helper function to MarketFunction
class, as it's needed in a few places
] | ||
|
||
|
||
class PredictPropabilityForQuestion(MarketFunction): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed otherwise it could easily be mistaken for GetMarketProbability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused - shouldn't this method be named something like PlaceBet
or BuyTokensForOutcome
? It's not really predicting anything, like the description says it allows the agent to purchase tokens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gabrielfior It's okay, just that Github hides the lines:
|
||
|
||
def get_binary_markets(market_type: MarketType) -> list[AgentMarket]: | ||
# Get the 5 markets that are closing soonest | ||
cls = market_type.market_class | ||
markets: list[AgentMarket] = cls.get_binary_markets( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Required change after bumping PMAT version
engine.register(Stop()) | ||
engine.register(GetMarkets(market_type=market_type)) | ||
engine.register(GetMarketProbability(market_type=market_type)) | ||
engine.register(Jsonify()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to give the agent this function, otherwise it would just stop after calling GetMarketProbability
, so the final output was:
>> GetMarketProbability(market_id="0x04f93bad589f989313bf3386f9dfa0f9354c9c25")
['0.43316548923547493']
Error: empty reply, aborting
def test_get_probability(market_type: MarketType) -> None: | ||
market_id = "0x0020d13c89140b47e10db54cbd53852b90bc1391" | ||
get_market_probability = GetMarketProbability(market_type=market_type) | ||
get_market_probability(market_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the assertion missing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes good point. I noticed that the way we calculate p_yes for closed omen markets is broken atm, so I made this task gnosis/prediction-market-agent-tooling#181
I've added an assertion here that will fail once that task is resolved, but I think that's fine.
m_json = json.loads(agent.history[-3]["content"]) | ||
m = MarketIDAndProbability.model_validate(m_json) | ||
market: AgentMarket = market_type.market_class.get_binary_market(m.market_id) | ||
assert market.p_yes == m.probability |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like this test a lot!
Just wondering - self.history
makes sense (I don't see another way to fetch microchain output - one could use on_iteration_end
and assign a variable, but don't think it would be much better).
This is a lot hackier though when compared with crewai (task.output.raw_content
, task.output.json_output
, etc) and autogen. So probably something to keep in mind when comparing those frameworks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a good point. I thought the same
Changes to microagent to prevent the agent from having to pass the 'market question' around to various Functions
GetMarkets
to return market questions and IDsGetMarketProbability
for getting the market p_yes from the market idBuyTokens
Functions take in market_id instead of market questionGetMarketProbability