Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
137 arbitrage agent #511
base: main
Are you sure you want to change the base?
137 arbitrage agent #511
Changes from 8 commits
dd8873a
66518be
3b59493
c04a3d0
72f54ba
433656d
2b69cbe
2104e4d
a33aeee
d8d2601
a3af608
6f1efbd
75d1b04
1677dcd
55aee24
4a7d6ac
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
Correct the logic in
market_to_bet_no
methodThe
market_to_bet_yes
method looks good. However, themarket_to_bet_no
method's logic doesn't align with its purpose.Update the condition in the
market_to_bet_no
method to comparecurrent_p_no
:This change ensures we're selecting the market with the higher "NO" probability, which is consistent with the method's name and purpose.
📝 Committable suggestion
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.
🛠️ Refactor suggestion
Consider refactoring for clarity, but current implementation is correct
The current implementation of
market_to_bet_no
is functionally correct, as it selects the market with the higher "YES" probability for a "NO" bet. However, for consistency and clarity, consider refactoring to directly compare "NO" probabilities.While the current implementation is correct, you might want to consider the following refactor for improved readability:
This change makes the logic more intuitive by directly comparing "NO" probabilities, which aligns with the method's name and purpose.
📝 Committable suggestion
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.
Add check for division by zero
The
split_bet_amount_between_yes_and_no
method correctly implements the formula for splitting the bet amount. However, it's important to handle the case where the denominator could be zero to prevent potential runtime errors.Add a check to ensure the denominator is not zero before performing the division:
This change will prevent division by zero errors and provide a clear error message when the bet amount cannot be split due to zero probabilities.
The rest of the implementation looks good and correctly calculates the split bet amounts.
📝 Committable suggestion
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.
🛠️ Refactor suggestion
Consider making the
model
attribute configurableThe
model
attribute is currently hardcoded to "gpt-4o-mini". To improve flexibility and allow for easier updates or testing with different models, consider making it a configurable parameter, either through the constructor or a configuration file.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 thought the plan was to go with the 'perfectly +vely/-vely correlated only" cases to begin with?
Looks like the implementation of
split_bet_amount_between_yes_and_no
also assumes perfect correlation, so I think you make them consistent by either:_build_chain
to only get perfectly correlated marketsI'd vote (2)
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 also like (2) better, but markets that are obviously strongly correlated, e.g. like in the test
only delivers 0.8 correlation.
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 ran it with gpt-4o instead of 4o-mini and it gives 1.0!!
Maybe use that instead? Should be very cheap to run these prompts. Have you checked?
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.
An alternative approach and (IMO simpler) is to prompt the agent to return a boolean, which is True if 'perfect or near-perfect correlation'. Then you don't need any numerical filtering based on some threshold. I say this, because we know that LLMs aren't the best at giving accurate numerical values!
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.
Sounds like a good approach, implemented
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.
Any reason you've made this agent omen-specific? Looks like the only thing you've used the sgh for is to get markets. So it looks like this could easily be changed to become market-platform agnostic
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 made it Omen-specific because the
related_markets
should be queryable via Pinecone (because of the TTA agent), and only Omen agents are stored there currently.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.
Oh I didn't realise that. Where does the indexing of omen markets in the pincone db happen? If it's not part of this agent, it goes against @kongzii's philosophy of "anyone should be able to run the agents (if they have appropriate api keys), and get the same kind of results".
Regardless, there doesn't seem to be a need to have any Omen dependencies here - you could just have a
if market_type != MarketType.Omen: raise ...
checkThere 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.
Indexing occurs below
prediction-market-agent/prediction_market_agent/agents/think_thoroughly_agent/think_thoroughly_agent.py
Line 190 in a1931b8
I don't think it prevents someone from running the agent - you just need Pinecone api keys and, on the first run, it will fill out Pinecone and have the same dataset as we do.
Makes sense to me, implementing this.
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.
Well it does if the user wants to run just this agent, and not the DeployableThinkThoroughlyAgent as well! I think we need to do
self.pinecone_handler.insert_all_omen_markets_if_not_exists(...)
somewhere hereThere 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.
Sure - added 55aee24
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.
Just had a look at what
pinecone_handler.update_markets
is doing. Is this what we want? Two things I'm not sure about:created_after = utcnow() - datetime.timedelta(days=7)
. Don't we want all open 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.
🛠️ Refactor suggestion
Consider adding error handling to the
load
methodThe
load
method initializes important components of the agent. To improve robustness, consider adding error handling to catch and log any exceptions that might occur during initialization ofsubgraph_handler
,pinecone_handler
, orchain
.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.
🛠️ Refactor suggestion
Implement actual market analysis or add a TODO comment.
The
answer_binary_market
method currently returns a fixed 50% probability with full confidence for all markets. This doesn't provide any real market analysis. If this is a placeholder, consider adding a TODO comment explaining the intended future implementation. If not, implement actual market analysis logic.Example:
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.
🛠️ Refactor suggestion
Consider making the temperature configurable.
The
_build_chain
method uses a hardcoded temperature of 0 for the ChatOpenAI model. To improve flexibility and allow for different levels of randomness in the model's output, consider making the temperature a configurable parameter. You could add it as a class attribute or a parameter to the method.Example:
This change would allow for easier experimentation with different temperature settings without modifying the code.
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.
🛠️ Refactor suggestion
Add error handling for chain invocation.
The
calculate_correlation_between_markets
method invokes the language model chain without any error handling. Consider adding a try-except block to catch and handle potential exceptions from the API call. This will improve the robustness of the method and prevent potential crashes due to network issues or API failures.Example implementation:
This change would make the method more robust to potential failures in the chain invocation.
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.
Incorrect arguments passed to
self.chain.invoke
In the
get_correlated_markets
method, you're passingmarket
andrelated_market
objects toself.chain.invoke
, whereas the chain expects strings containing the market questions.Apply this diff to fix the arguments:
📝 Committable suggestion
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.
This seems like something that should be configurable at the top level
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.
Implemented
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.
Buying [market1: yes, market2: no] assumes positive correlation between markets. I think you need to have some logic that chooses the directions based on correlation
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.
Positive correlation is assumed here -> https://github.com/gnosis/prediction-market-agent/pull/511/files/433656d955694050935c662e1574f80c552d3cc7#diff-1b9d8df9eb7cb55918e642ebc0f26902d5211d4946a8c4d0454e9889dd05c371R138-R140
I didn't implement logic for the negatively correlated case (it was on an earlier version), I think positive correlation is a good start. Also, negative correlation is harder to determine, e.g.
Trump president vs Kamala president
is 100% negatively correlated but not trivial for LLM to tell, hence preferred to stick to 100% correlation case.Happy with suggestions though.
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.
Oh okay. Fwiw I think it's okay if the LLMs find it hard to determine that, as long as there aren't false positives.
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.
Going with your
bool
suggestion from the other thread.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.
🛠️ Refactor suggestion
Consider utilizing or removing unused parameters in
build_trades
.The
build_trades
method currently ignores theanswer
andexisting_position
parameters. If these parameters are not needed for the arbitrage strategy, consider removing them to avoid confusion. If they could be useful, consider incorporating them into your trade building logic.If the parameters are not needed:
If you want to utilize these parameters, consider adjusting the trade amount based on the answer's confidence or the existing position.
The overall logic of the method for building trades based on correlated markets with potential profit is correct.
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 by this, if you expect high correlation with the other question shouldn't this be either:
"Will Kamala Harris be the US president in 2025?"
or"Will Kamala Harris become the US president in 2024?"
because the election is in Nov 2024
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.
Oh scrap that sorry, elected president takes office in Jan 2025. Forgot!
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.
😆