Monitor from GCP deployment #54
Conversation
WalkthroughThe update significantly enhances the prediction market agent tooling, focusing on deployment and monitoring enhancements. It introduces functionalities to fetch and display deployed agent data from Google Cloud Platform (GCP) within the monitor app, supports monitoring capabilities with alerts, and ensures the integration is both reliable and scalable. Additionally, it refines data handling in the Changes
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 Configration File (
|
| start_time=start_time, omen_public_key=omen_public_key | ||
| ), | ||
| ) | ||
| for omen_public_key in settings.OMEN_PUBLIC_KEYS |
There was a problem hiding this comment.
Changed to allow multiple keys as with manifold
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
poetry.lockis excluded by:!**/*.lockpyproject.tomlis excluded by:!**/*.toml
Files selected for processing (8)
- prediction_market_agent_tooling/benchmark/utils.py (2 hunks)
- prediction_market_agent_tooling/deploy/agent.py (4 hunks)
- prediction_market_agent_tooling/deploy/gcp/utils.py (3 hunks)
- prediction_market_agent_tooling/monitor/markets/manifold.py (1 hunks)
- prediction_market_agent_tooling/monitor/markets/omen.py (1 hunks)
- prediction_market_agent_tooling/monitor/monitor.py (3 hunks)
- prediction_market_agent_tooling/monitor/monitor_app.py (3 hunks)
- prediction_market_agent_tooling/tools/utils.py (2 hunks)
Additional comments: 22
prediction_market_agent_tooling/tools/utils.py (3)
- 3-3: The import of
datetimeis correctly added to support the new functionality introduced in this file.- 6-6: The import of
pytzis correctly added to support timezone handling in the newadd_timezone_validatorfunction.- 68-71: The
add_timezone_validatorfunction correctly checks if thedatetimeobject has a timezone set. If not, it sets the timezone to UTC usingpytz.UTC. This ensures that all datetime objects are timezone-aware, which is a good practice for avoiding issues related to timezone differences.prediction_market_agent_tooling/monitor/markets/manifold.py (3)
- 19-24: The new
manifold_user_idproperty correctly retrieves themanifold_user_idfrom themonitor_config, ensuring it is notNoneusing thecheck_not_noneutility. This is a good practice for ensuring the configuration is correctly set up before proceeding with operations.- 34-40: The
get_all_deployed_agents_gcpstatic method correctly retrieves all deployed agents on GCP that are of theMarketType.MANIFOLDtype. This method enhances the monitoring capabilities by allowing the retrieval of specific agent types deployed on GCP.- 46-57: The
from_monitor_settingsmethod has been updated to return a list ofDeployedManifoldAgentinstances based onMonitorSettings. This update correctly utilizes theMonitorConfigfor initializing each agent, ensuring that the start time andmanifold_user_idare correctly set. This is a significant improvement for creating agent instances based on monitoring settings.prediction_market_agent_tooling/monitor/markets/omen.py (3)
- 18-25: The new
wallet_addressproperty correctly converts theomen_public_keyfrom themonitor_configto a checksum address usingWeb3.to_checksum_address. This ensures that the wallet address is correctly formatted for further operations.- 35-41: The
get_all_deployed_agents_gcpstatic method correctly retrieves all deployed agents on GCP that are of theMarketType.OMENtype. This method enhances the monitoring capabilities by allowing the retrieval of specific agent types deployed on GCP.- 47-57: The
from_monitor_settingsmethod has been updated to return a list ofDeployedOmenAgentinstances based onMonitorSettings. This update correctly utilizes theMonitorConfigfor initializing each agent, ensuring that the start time andomen_public_keyare correctly set. This is a significant improvement for creating agent instances based on monitoring settings.prediction_market_agent_tooling/deploy/gcp/utils.py (2)
- 130-133: The
list_gcp_functionsfunction correctly lists all GCP functions using theFunctionServiceClient. This refactoring improves clarity and organization by separating the listing and retrieval functionalities.- 127-140: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [136-148]
The
get_gcp_functionfunction has been updated to use thelist_gcp_functionsfor retrieving the list of functions and then filtering to find the specific function by name. This change maintains the functionality while improving the organization of the code.prediction_market_agent_tooling/monitor/monitor_app.py (3)
- 31-46: The reordering of parameters in the
get_deployed_agentsfunction improves clarity by aligning the parameter order with the logical flow of information (market type, start time, settings).- 49-77: The addition of the
get_open_and_resolved_marketsfunction enhances the logic for loading agents and markets based on the selected market type. This function correctly separates the retrieval of open and resolved markets, improving modularity and maintainability of the code.- 122-148: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [111-143]
The adjustments in the
monitor_appfunction to utilize theget_deployed_agentsandget_open_and_resolved_marketsfunctions based on the selected market type improve the user interface experience by dynamically loading relevant data. This is a good practice for creating responsive and user-friendly interfaces.prediction_market_agent_tooling/deploy/agent.py (3)
- 6-9: The addition of imports for
datetime,git, andBaseModelfrompydanticsupports the new functionality introduced in this file, specifically theMonitorConfigclass and the enhancements to thedeploy_gcpfunction.- 30-51: The
MonitorConfigclass correctly defines a Pydantic model for monitor configurations, including validation formanifold_user_idandomen_public_keybased on themarket_type. This is a good practice for ensuring configurations are valid and complete before proceeding with deployment.- 118-145: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [103-141]
The
deploy_gcpfunction has been updated to include amonitor_configparameter, enhancing the setup of environment variables for monitoring. The validation of the monitor configuration and the inclusion of monitor configuration data in environment variables are correctly implemented, improving the deployment process's flexibility and configurability.prediction_market_agent_tooling/monitor/monitor.py (3)
- 16-22: The addition of imports for
MonitorConfig,MarketType, andlist_gcp_functionssupports the new functionality introduced in this file, specifically the enhancements related to agent deployment and monitoring.- 32-34: The introduction of the
LOAD_FROM_GCPboolean flag and the modification ofOMEN_PUBLIC_KEYSto a list are aligned with the PR objectives of enhancing monitoring capabilities and configuration management. These changes improve the flexibility and configurability of the monitoring setup.- 51-77: The updates related to agent deployment and monitoring, including the
get_all_deployed_agents_gcpmethod and the adjustments in the calculation of profit, are correctly implemented. These changes enhance the tooling's ability to interact with and monitor deployed agents on GCP, contributing to the overall improvement of the monitoring capabilities.prediction_market_agent_tooling/benchmark/utils.py (2)
- 9-12: The import of
add_timezone_validatorand other utilities fromtools/utils.pyis correctly done.This import is necessary for the new validation logic introduced in the
Marketclass, specifically for ensuring datetime objects are timezone-aware.
- 54-59: The new field validator for
outcomePricesis correctly implemented.This validator ensures that
outcomePrices, if provided, must exactly contain 2 elements. This is a crucial validation for maintaining data integrity, especially when dealing with market outcome prices which typically come in pairs (e.g., yes/no outcomes).
| import requests | ||
| from pydantic import BaseModel, validator | ||
| from pydantic import BaseModel, field_validator |
There was a problem hiding this comment.
The import statement for field_validator seems to be incorrect.
- from pydantic import BaseModel, field_validator
+ from pydantic import BaseModel
+ from pydantic.class_validators import validator as field_validatorThe correct import for using field validators in Pydantic is from pydantic.class_validators. This change ensures that the field_validator decorator is correctly imported and can be used for validating fields in the Market class.
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.
| from pydantic import BaseModel, field_validator | |
| from pydantic import BaseModel | |
| from pydantic.class_validators import validator as field_validator |
| if value.tzinfo is None: | ||
| value = value.replace(tzinfo=pytz.UTC) | ||
| return value | ||
| _add_timezone_validator = field_validator("created_time")(add_timezone_validator) |
There was a problem hiding this comment.
The application of the field_validator decorator for created_time using add_timezone_validator is incorrect.
- _add_timezone_validator = field_validator("created_time")(add_timezone_validator)
+ @field_validator("created_time", pre=True)
+ def _add_timezone_validator(cls, v):
+ return add_timezone_validator(v)The original line attempts to directly apply field_validator as a decorator, which is not the correct usage. Instead, you should define a method for the validation and then apply the field_validator decorator to this method, passing the validation function as the body. This ensures that created_time is correctly validated to be timezone-aware.
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.
| _add_timezone_validator = field_validator("created_time")(add_timezone_validator) | |
| @field_validator("created_time", pre=True) | |
| def _add_timezone_validator(cls, v): | |
| return add_timezone_validator(v) |
| FunctionServiceClient, | ||
| ) | ||
| from google.cloud.functions_v2.types.functions import Function | ||
| from google.cloud.secretmanager import SecretManagerServiceClient | ||
|
|
||
|
|
||
| def gcloud_deploy_cmd( |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [60-64]
The use of subprocess.run with shell=True in the export_requirements_from_toml function is flagged by static analysis as dangerous due to the potential for shell injection. It's recommended to use shell=False and pass the command as a list to mitigate this risk.
- subprocess.run(f"poetry export -f requirements.txt --without-hashes --output {output_file}", shell=True, check=True)
+ subprocess.run(["poetry", "export", "-f", "requirements.txt", "--without-hashes", "--output", output_file], shell=False, check=True)
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- prediction_market_agent_tooling/monitor/monitor_app.py (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- prediction_market_agent_tooling/monitor/monitor_app.py
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- prediction_market_agent_tooling/deploy/agent.py (4 hunks)
- prediction_market_agent_tooling/monitor/monitor_app.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- prediction_market_agent_tooling/monitor/monitor_app.py
Additional comments: 2
prediction_market_agent_tooling/deploy/agent.py (2)
- 6-9: The addition of imports for
datetime,git, andBaseModelfrompydanticis appropriate for the new features being introduced. Ensure that these libraries are included in the project's dependencies.- 119-146: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [104-142]
The update to the
deploy_gcpfunction to include amonitor_configparameter and the enhanced environment variable setup with monitor configuration data are significant improvements. This allows for more flexible and detailed configuration of the deployment process. However, ensure that themodel_dumpmethod called onmonitor_configexists and is correctly implemented in theMonitorConfigclass or its parent classes. If it's a custom method, it might be a typo or require implementation. If it's meant to bedict(monitor_config), this should be corrected.
| class MonitorConfig(BaseModel): | ||
| LABEL_PREFIX: t.ClassVar[str] = "monitor_config_" | ||
|
|
||
| start_time: datetime | ||
| end_time: t.Optional[ | ||
| datetime | ||
| ] = None # TODO: If we want end time, we need to store agents somewhere, not just query them from functions. | ||
| manifold_user_id: str | None = None | ||
| omen_public_key: str | None = None | ||
|
|
||
| _add_timezone_validator = field_validator("start_time")(add_timezone_validator) | ||
|
|
||
| def validate_monitor_config(self, market_type: MarketType) -> None: | ||
| if market_type == MarketType.MANIFOLD and not self.manifold_user_id: | ||
| raise ValueError( | ||
| "You must provide a manifold_user_id when deploying a Manifold agent" | ||
| ) | ||
|
|
||
| if market_type == MarketType.OMEN and not self.omen_public_key: | ||
| raise ValueError( | ||
| "You must provide a omen_public_key when deploying a Omen agent" | ||
| ) |
There was a problem hiding this comment.
The MonitorConfig class introduces field validators and conditional validation based on the market type. This is a good practice for ensuring data integrity and providing clear error messages for configuration issues. However, consider adding a brief docstring to the class and its methods to improve maintainability and understandability for future developers.
class MonitorConfig(BaseModel):
+ """
+ Configuration class for monitoring settings, including validators for start time and specific user IDs based on market type.
+ """
LABEL_PREFIX: t.ClassVar[str] = "monitor_config_"
...
def validate_monitor_config(self, market_type: MarketType) -> None:
+ """
+ Validates the monitor configuration based on the provided market type.
+ Raises ValueError if required fields are missing for the specified market type.
+ """
...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.
| class MonitorConfig(BaseModel): | |
| LABEL_PREFIX: t.ClassVar[str] = "monitor_config_" | |
| start_time: datetime | |
| end_time: t.Optional[ | |
| datetime | |
| ] = None # TODO: If we want end time, we need to store agents somewhere, not just query them from functions. | |
| manifold_user_id: str | None = None | |
| omen_public_key: str | None = None | |
| _add_timezone_validator = field_validator("start_time")(add_timezone_validator) | |
| def validate_monitor_config(self, market_type: MarketType) -> None: | |
| if market_type == MarketType.MANIFOLD and not self.manifold_user_id: | |
| raise ValueError( | |
| "You must provide a manifold_user_id when deploying a Manifold agent" | |
| ) | |
| if market_type == MarketType.OMEN and not self.omen_public_key: | |
| raise ValueError( | |
| "You must provide a omen_public_key when deploying a Omen agent" | |
| ) | |
| class MonitorConfig(BaseModel): | |
| """ | |
| Configuration class for monitoring settings, including validators for start time and specific user IDs based on market type. | |
| """ | |
| LABEL_PREFIX: t.ClassVar[str] = "monitor_config_" | |
| start_time: datetime | |
| end_time: t.Optional[ | |
| datetime | |
| ] = None # TODO: If we want end time, we need to store agents somewhere, not just query them from functions. | |
| manifold_user_id: str | None = None | |
| omen_public_key: str | None = None | |
| _add_timezone_validator = field_validator("start_time")(add_timezone_validator) | |
| def validate_monitor_config(self, market_type: MarketType) -> None: | |
| """ | |
| Validates the monitor configuration based on the provided market type. | |
| Raises ValueError if required fields are missing for the specified market type. | |
| """ | |
| if market_type == MarketType.MANIFOLD and not self.manifold_user_id: | |
| raise ValueError( | |
| "You must provide a manifold_user_id when deploying a Manifold agent" | |
| ) | |
| if market_type == MarketType.OMEN and not self.omen_public_key: | |
| raise ValueError( | |
| "You must provide a omen_public_key when deploying a Omen agent" | |
| ) |
| @@ -83,6 +118,28 @@ def {entrypoint_function_name}(request) -> str: | |||
|
|
|||
| gcp_fname = gcp_fname or self.get_gcloud_fname(market_type) | |||
|
|
|||
| # For labels, only hyphens (-), underscores (_), lowercase characters, and numbers are allowed in values. | |||
| labels = (labels or {}) | { | |||
There was a problem hiding this comment.
Why is something a label vs an env var?
There was a problem hiding this comment.
Labels are useful in UI:
But they are very limited in values they can store (-, _, lower-case characters), so my idea was to have stuff that can be useful to filter by in labels and otherwise environment.
There was a problem hiding this comment.
But as I'm working on a new implementation, it might be nicer just to have everything in env vars, less confusion. People are free to add to labels whatever they want during deployment anyway.
| return [ | ||
| DeployedOmenAgent( | ||
| name="OmenAgent", | ||
| agent_class=DeployedAgent.__name__, |
There was a problem hiding this comment.
Do you mean DeployedOmenAgent?
There was a problem hiding this comment.
Actually, I meant DeployableAgent 😄 Thanks. This is populated automatically by name of the deployed agent class, for example DeployableAgentER_EvoGPT3, but here it's loaded just by the public-key/api-key, so I fill it with the base class name.
| return [ | ||
| DeployedManifoldAgent( | ||
| name="ManifoldAgent", | ||
| agent_class=DeployedAgent.__name__, |
There was a problem hiding this comment.
Do you mean DeployedManifoldAgent?
| end_time: t.Optional[ | ||
| datetime | ||
| ] = None # TODO: If we want end time, we need to store agents somewhere, not just query them from functions. | ||
| manifold_user_id: str | None = None |
There was a problem hiding this comment.
Imo we should avoid having generic classes that contain stuff from market-specific implementations
There was a problem hiding this comment.
Agreed, working on it.
| try: | ||
| agent = DeployedAgent( | ||
| name=function.name.split("/")[-1], | ||
| agent_class=function.service_config.environment_variables[ |
There was a problem hiding this comment.
I don't think agent_class should have to be a property of DeployedAgent.
I feel like we should have some specialised function from_gcp_function / from_dict (and maybe you'll need to_dict too) that you call here:
agent_class = function.service_config.environment_variables[AGENT_CLASS_KEY]
cls = DEPLOYED_AGENT_TYPE_MAP[agent_class]
agent = cls.from_gcp_function(...)
then MonitorConfig won't need to be a thing.
There was a problem hiding this comment.
It's a little confusing, because right now, we have DeployableAgent and DeployedAgent, how it works is that in EVO repo, I subclass DeployableAgent and then deploy them from there:
class DeployableAgentER(DeployableAgent):
agent: AbstractBenchmarkedAgent
...
class DeployableAgentER_EvoGPT3(DeployableAgentER):
agent = EvoAgent(model="gpt-3.5-turbo-0125")
class DeployableAgentER_OlasEmbeddingOA(DeployableAgentER):
agent = OlasAgent(model="gpt-3.5-turbo-0125", embedding_model=EmbeddingModel.openai)
...
chosen_agent_class().deploy_gcp(...)And agent_class (maybe it needs a better name, deployable_agent_class?) is actually name of the subclassed DeployableAgent.
What is nice right now, that we can use monitor.py in PMAT, without need of EVO-specific code. So PMAT can be universally used to monitor everything.
If we would want something as
agent_class = function.service_config.environment_variables[AGENT_CLASS_KEY]
cls = DEPLOYED_AGENT_TYPE_MAP[agent_class]
agent = cls.from_gcp_function(...)
then this needs to be coded in EVO, not universally here, doesn't it?
But now based on the comment above, I see you wanted agent_class to be DeployedOmenAgent/DeployedManifoldAgent.
There is something as you describe already, DeployedAgent.get_all_deployed_agents_gcp(), I can refactor that to allow to init just a single agent too.
then MonitorConfig won't need to be a thing.
I think my main motivation for MonitorConfig was because of deployment code in EVO, right now there is
chosen_agent_class().deploy_gcp(
...,
monitor_config=MonitorConfig( # I want to change this to subclasses OmenMonitorConfig, ManifoldMonitorConfig, but the idea remains
start_time=start_time or datetime.utcnow(),
manifold_user_id=get_authenticated_user(gcp_get_secret_value(manifold_api_key_secret_name)).id,
omen_public_key=bet_from_address,
)
)How would you design it without MonitorConfig? Wouldn't we need to add market-specific optional arguments to deploy_gcp?
There was a problem hiding this comment.
Hmm maybe I can keep something as MonitorConfig as input argument for deploy_gcp, but keep it in flat in DeployedOmenAgent?
There was a problem hiding this comment.
And agent_class (maybe it needs a better name, deployable_agent_class?) is actually name of the subclassed DeployableAgent.
Ah sorry, that's clear now!
There was a problem hiding this comment.
If we would want something as ... then this needs to be coded in EVO, not universally here, doesn't it?
Shouldn't have to be any specialisation in the DeployableAgent subclasses - only in the DeployedAgent subclasses. But can see from from_all_gcp_functions that you've already implemented this, nice!
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- prediction_market_agent_tooling/monitor/markets/manifold.py (2 hunks)
- prediction_market_agent_tooling/monitor/markets/omen.py (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- prediction_market_agent_tooling/monitor/markets/manifold.py
- prediction_market_agent_tooling/monitor/markets/omen.py
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- prediction_market_agent_tooling/deploy/agent.py (4 hunks)
- prediction_market_agent_tooling/monitor/markets/manifold.py (1 hunks)
- prediction_market_agent_tooling/monitor/markets/omen.py (1 hunks)
- prediction_market_agent_tooling/monitor/monitor.py (3 hunks)
- prediction_market_agent_tooling/monitor/monitor_app.py (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- prediction_market_agent_tooling/monitor/markets/manifold.py
- prediction_market_agent_tooling/monitor/monitor_app.py
Additional comments: 15
prediction_market_agent_tooling/monitor/markets/omen.py (6)
- 4-11: The addition of imports for
Function,BaseModel,DeployableAgent, and others aligns with the PR's objectives to enhance monitoring capabilities and refactor code for clarity. These imports are essential for the new functionalities introduced in this file.- 22-23: The
DeployedOmenAgentParamsclass is introduced to encapsulate parameters specific to deployed Omen agents. UsingBaseModelfrom Pydantic for parameter validation is a good practice, ensuring thatomen_public_keyis always a validChecksumAddress.- 27-27: Changing the
wallet_addressattribute toomen_public_keywith a nullable type inDeployedOmenAgentis a meaningful update that clarifies the purpose of the attribute. It's important to ensure that all references towallet_addressin the codebase are updated toomen_public_keyto maintain consistency.Verification successful
Given the absence of any output from the search for
wallet_addressin Python files, it appears that all references towallet_addresshave been successfully updated toomen_public_keyin the codebase. This suggests that the change was thoroughly applied, and there are no inconsistencies with the update as per the review comment.* 30-37: The `get_resolved_bets` method's update to handle `omen_public_key` and `end_time` is crucial for fetching resolved bets accurately. This change aligns with the PR's goal to enhance monitoring capabilities. Ensure that the `get_bets` function called within this method is updated accordingly to accept these parameters.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to `wallet_address` that should be updated. rg --type py 'wallet_address'Length of output: 29
Verification successful
The
get_betsfunction has been updated to acceptomen_public_keyandend_timeas parameters, as evidenced by its usage in both thetest_omen.pyandomen.pyfiles. This confirms that the changes align with the PR's goal to enhance monitoring capabilities by accurately fetching resolved bets.* 45-53: The `from_monitor_settings` method's update to create instances with `omen_public_key` is a logical change that supports the PR's objectives. It's important to ensure that the `MonitorSettings` class and its usage are updated to include `OMEN_PUBLIC_KEYS` as a valid attribute.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the `get_bets` function is updated to accept `omen_public_key` and `end_time`. ast-grep --lang python --pattern $'get_bets($_, $_, $_)'Length of output: 839
Verification successful
The search results confirm that
OMEN_PUBLIC_KEYSis indeed defined and used within theprediction_market_agent_toolingproject. Specifically, it is defined inprediction_market_agent_tooling/monitor/monitor.pyas an attribute of what appears to be a settings or configuration class, with a default value of an empty list. This attribute is then used in a conditional statement within the same file, and its values are iterated over inprediction_market_agent_tooling/monitor/markets/omen.py. This confirms thatOMEN_PUBLIC_KEYSis a valid attribute as mentioned in the review comment, and it is utilized in a manner consistent with the PR's objectives.Given this information, the review comment regarding the update to the
from_monitor_settingsmethod to create instances withomen_public_keyand the importance of ensuring that theMonitorSettingsclass (or a similar settings/configuration class as indicated by the context) includesOMEN_PUBLIC_KEYSas a valid attribute is verified to be accurate and consistent with the codebase.* 55-63: The addition of the `from_all_gcp_functions` method is a significant enhancement that allows for retrieving all deployed agents on GCP based on specific filters. This method leverages the class method inheritance from `DeployedAgent` and applies a filter specific to Omen agents. This is a good use of polymorphism and aligns with the PR's goal to improve monitoring capabilities.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that `MonitorSettings` includes `OMEN_PUBLIC_KEYS` and is used correctly. ast-grep --lang python --pattern $'class MonitorSettings { $$$ OMEN_PUBLIC_KEYS: $_ $$$ }'Length of output: 436
Script:
#!/bin/bash # Search for `OMEN_PUBLIC_KEYS` across all Python files to verify its presence and usage. rg "OMEN_PUBLIC_KEYS" --type pyLength of output: 372
prediction_market_agent_tooling/deploy/agent.py (3)
- 19-37: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [6-34]
The addition of imports for
datetime,git,DeployedManifoldAgent,DeployedOmenAgent, and their respective parameter classes, along with the introduction of constantsMARKET_TYPE_KEY,REPOSITORY_KEY, andCOMMIT_KEY, are well-aligned with the PR's objectives to enhance deployment configuration and management. These changes lay the groundwork for the subsequent modifications in thedeploy_gcpfunction.
- 104-157: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [86-150]
The modifications to the
deploy_gcpfunction, including the addition ofstart_timeandmonitor_paramsparameters, significantly enhance the deployment process by allowing for more granular control over deployment configurations. The logic to instantiatemonitor_agentbased onmarket_typeandmonitor_paramsis a robust addition that supports the PR's goal of improving monitoring capabilities. However, ensure that theDeployableAgentclass and its subclasses are updated to handle these new parameters correctly.
- 152-154: Enhancing the environment variable setup with monitor configuration data is a crucial improvement that supports the PR's objectives. This change ensures that deployed agents have access to necessary configuration data at runtime, enhancing the tooling's ability to interact with and monitor agents on GCP. It's important to validate that the
model_dump_prefixedmethod correctly prefixes environment variables to avoid conflicts.prediction_market_agent_tooling/monitor/monitor.py (6)
- 9-22: The addition of imports for
Function,add_timezone_validator, and adjustments in existing imports, along with the introduction of theLOAD_FROM_GCPflag and modifications toOMEN_PUBLIC_KEYS, are well-aligned with the PR's objectives to enhance monitoring capabilities. These changes support the new functionalities introduced in this file and improve the tooling's ability to interact with and monitor agents on GCP.- 35-38: The introduction of the
has_manual_agentsproperty in theMonitorSettingsclass is a useful addition that provides a quick way to check if there are any manually configured agents. This aligns with the PR's goal of enhancing monitoring capabilities by allowing for more flexible configuration options.- 44-58: The modifications and additions to the
DeployedAgentclass, including new properties and the introduction of the_add_timezone_validatorfield validator, significantly enhance the class's functionality. These changes support the PR's objectives by improving the tooling's ability to accurately monitor deployed agents, particularly with the correct handling of timezones. Ensure that theadd_timezone_validatorfunction is correctly implemented and tested.- 71-100: The introduction of methods
from_gcp_functionandfrom_all_gcp_functionsin theDeployedAgentclass is a significant enhancement that allows for retrieving and parsing deployed agent information from GCP functions. These methods support the PR's goal of improving monitoring capabilities by enabling more efficient and accurate retrieval of deployed agents. It's important to ensure that these methods are correctly implemented and handle edge cases gracefully.Verification successful
The implementation and error handling in
from_gcp_functionandfrom_all_gcp_functionswithin theDeployedAgentclass and its subclasses are correctly aligned with the goal of improving monitoring capabilities by enabling more efficient and accurate retrieval of deployed agents. The methods are present and implemented as described, supporting the initial review comment.* 115-115: The adjustment in the calculation of profit in the `monitor_agent` function is a necessary update that aligns with the PR's objectives to enhance monitoring capabilities. Ensuring that the profit calculation is accurate is crucial for providing meaningful insights into the performance of deployed agents. It's important to validate that the `ResolvedBet` data model and its `profit` attribute are correctly implemented. * 119-124: The display of agent information in the user interface, including the agent's name, deployable agent class, and start time, is a valuable addition that enhances the user experience. These changes support the PR's goal of improving the tooling's usability and efficiency by providing users with more detailed insights into deployed agents.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the correct implementation and error handling in `from_gcp_function` and `from_all_gcp_functions`. ast-grep --lang python --pattern $'class DeployedAgent { $$$ from_gcp_function($_) { $$$ } $$$ from_all_gcp_functions($_) { $$$ } $$$ }'Length of output: 482
Script:
#!/bin/bash # Attempt a broader search for the methods within any class to verify their existence and implementation. rg --type py 'def from_gcp_function' -A 10 -B 10 rg --type py 'def from_all_gcp_functions' -A 10 -B 10Length of output: 7270
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- prediction_market_agent_tooling/deploy/agent.py (4 hunks)
- prediction_market_agent_tooling/deploy/constants.py (1 hunks)
- prediction_market_agent_tooling/monitor/markets/manifold.py (1 hunks)
- prediction_market_agent_tooling/monitor/markets/omen.py (1 hunks)
Files skipped from review due to trivial changes (1)
- prediction_market_agent_tooling/deploy/constants.py
Files skipped from review as they are similar to previous changes (2)
- prediction_market_agent_tooling/deploy/agent.py
- prediction_market_agent_tooling/monitor/markets/manifold.py
Additional comments: 6
prediction_market_agent_tooling/monitor/markets/omen.py (6)
- 4-5: The addition of imports for
Function,BaseModel, andDeployableAgentaligns with the enhancements made in theDeployedOmenAgentclass and its methods. These imports are necessary for the new functionality introduced in this PR.- 19-21: The
DeployedOmenAgentParamsclass has been introduced with a single attributeomen_public_keyof typeChecksumAddress. This change is consistent with the shift towards usingomen_public_keyinstead ofwallet_address, enhancing clarity and specificity in the codebase.- 24-24: The
omen_public_keyattribute in theDeployedOmenAgentclass has been made nullable (ChecksumAddress | None = None). This change accommodates scenarios where anomen_public_keymight not be available, increasing the flexibility of theDeployedOmenAgentinstances.- 27-34: The
get_resolved_betsmethod has been updated to handleomen_public_keyandend_time. This method now correctly fetches bets based on theomen_public_keyand the specified time range, enhancing the functionality of theDeployedOmenAgentclass. However, ensure that theget_betsfunction and theResolvedBet.to_generic_resolved_bet()method are equipped to handle the potential absence ofomen_public_keyand the implications of the nullable type.- 42-50: The
from_monitor_settingsstatic method has been updated to create instances ofDeployedOmenAgentwithomen_public_key. This method now properly utilizes theWeb3.to_checksum_addressfunction to ensure theomen_public_keyis in the correct format. This change is crucial for maintaining consistency and correctness in handling Ethereum addresses.- 52-60: The addition of the
from_all_gcp_functionsclass method is a significant enhancement, allowing for the retrieval of all deployed agents on GCP for the Omen market type. This method leverages the superclass method while applying a specific filter based on theMARKET_TYPE_KEYandMarketType.OMEN.value, which is a good use of inheritance and polymorphism. Ensure that the superclass methodfrom_all_gcp_functionsis designed to handle the filtering logic correctly and that thefilter_function is thoroughly tested, especially considering its lambda implementation.
| manifold_user_id: str | ||
|
|
||
|
|
||
| class DeployedManifoldAgent(DeployedAgent): | ||
| manifold_user_id: str | None = None |
There was a problem hiding this comment.
How come this is now optional?
There was a problem hiding this comment.
Because monitor params were optional in deploy_gcp, I had to make this optional, but because you were right in the other comment, I made it non-optional now 😄
| try: | ||
| agent = DeployedAgent( | ||
| name=function.name.split("/")[-1], | ||
| agent_class=function.service_config.environment_variables[ |
There was a problem hiding this comment.
And agent_class (maybe it needs a better name, deployable_agent_class?) is actually name of the subclassed DeployableAgent.
Ah sorry, that's clear now!
| try: | ||
| agent = DeployedAgent( | ||
| name=function.name.split("/")[-1], | ||
| agent_class=function.service_config.environment_variables[ |
There was a problem hiding this comment.
If we would want something as ... then this needs to be coded in EVO, not universally here, doesn't it?
Shouldn't have to be any specialisation in the DeployableAgent subclasses - only in the DeployedAgent subclasses. But can see from from_all_gcp_functions that you've already implemented this, nice!
| COMMIT_KEY: git.Repo(search_parent_directories=True).head.object.hexsha, | ||
| } | ||
|
|
||
| monitor_agent = ( |
There was a problem hiding this comment.
This last bit of if-elsing - I think we should be able to get rid of this too! Since the public key for the given market type (e.g. wallet address or manifold user id) needs to be specified in the env vars of the deployed agent anyway, why don't we just use this fact and parse the env vars to get these values?
So a new method on the DeployedAgent base class
class DeployedAgent(BaseModel):
# ...
def from_env_vars():
raise NotImplementedError("This method must be implemented by the subclass")
and e.g. the specialisation for omen
class DeployedOmenAgent(DeployedAgent):
# ...
def from_env_vars(
gcp_fname: str,
deployableagent_class_name: str,
start_time: datetime,
env_vars: dict[str, str],
) -> "DeployedOmenAgent":
if "OMEN_PUBLIC_KEY" not in env_vars:
raise ValueError("OMEN_PUBLIC_KEY not found in env_vars")
return DeployedOmenAgent(
name=gcp_fname,
deployableagent_class_name=deployableagent_class_name,
start_time=start_time,
omen_public_key=env_vars["OMEN_PUBLIC_KEY"],
)
So now we can just do
cls = DEPLOYED_AGENT_TYPE_MAP[market_type]
monitor_agent = cls.from_env_vars(env_vars, ...)
and we don't need monitor_params
There was a problem hiding this comment.
That could work! I did it a bit differently, but the idea is the same, we are already using APIKeys through the code, so why not here, check it out please! 🙏 In this way, it should be more type-friendly.
Also, I added SecretStr, which is Pydantic's special type. The downside is that we need to do get_secret_value to get the actual value in the code, but the benefit is that if we print APIKeys in logs or anywhere, secrets are automatically hidden behind "***"! And we can also differentiate between secrets and non-secrets at runtime!
I'm in no rush to merge this, so I will wait for you.
There was a problem hiding this comment.
Yes this looks great!
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (8)
- examples/cloud_deployment/gcp/deploy.py (2 hunks)
- prediction_market_agent_tooling/config.py (2 hunks)
- prediction_market_agent_tooling/deploy/agent.py (3 hunks)
- prediction_market_agent_tooling/deploy/gcp/utils.py (3 hunks)
- prediction_market_agent_tooling/markets/manifold/api.py (1 hunks)
- prediction_market_agent_tooling/monitor/markets/manifold.py (2 hunks)
- prediction_market_agent_tooling/monitor/markets/omen.py (2 hunks)
- prediction_market_agent_tooling/monitor/monitor.py (3 hunks)
Files skipped from review as they are similar to previous changes (3)
- prediction_market_agent_tooling/deploy/gcp/utils.py
- prediction_market_agent_tooling/monitor/markets/manifold.py
- prediction_market_agent_tooling/monitor/monitor.py
Additional comments: 18
examples/cloud_deployment/gcp/deploy.py (4)
- 4-5: The addition of imports for
SecretStrfrompydantic.typesandWeb3fromweb3is appropriate for enhancing security and data handling within the deployment script. UsingSecretStrfor sensitive information ensures that secrets are not accidentally logged or exposed.- 7-13: The import of
APIKeysandPrivateKeyfrom theprediction_market_agent_tooling.configandgtypesmodules, respectively, aligns with the objective of securely managing API keys and private keys. This change supports the secure handling of sensitive information, adhering to best practices for security.- 35-44: The update to use
SecretStrandPrivateKeyobjects forMANIFOLD_API_KEYandBET_FROM_PRIVATE_KEYwithin theapi_keysinstantiation is a significant improvement in terms of security. By leveraging these types, the code ensures that sensitive information is handled securely, reducing the risk of accidental exposure.- 39-39: The comment regarding GCP deployment requirements provides valuable context for developers, explaining the necessity of storing sensitive information like passwords and API keys in Secret Manager. This is a good practice for cloud deployments, ensuring that secrets are managed securely and are not hardcoded in the source code.
prediction_market_agent_tooling/config.py (3)
- 3-3: The import of
SecretStrfrompydantic.typesis a positive change, enabling the secure handling of sensitive information such as API keys within the configuration management.- 23-28: Changing the type of
MANIFOLD_API_KEYfromstrtoSecretStrand updating the return type of themanifold_api_keymethod toSecretStrenhances the security of API key management. This change ensures that the API key is treated as sensitive information throughout its lifecycle.- 49-61: The introduction of
model_dump_publicandmodel_dump_secretsmethods is a thoughtful addition for managing configuration data. These methods allow for a clear separation between public information and secrets, facilitating safer handling and potential serialization of configuration data without exposing sensitive information.prediction_market_agent_tooling/monitor/markets/omen.py (5)
- 1-11: The addition of imports for
Function,APIKeys,MARKET_TYPE_KEY, andMarketTypeis necessary for the new functionalities introduced in theDeployedOmenAgentclass. These imports support the enhanced monitoring logic and the interaction with GCP functions.- 20-26: Changing the
wallet_addressproperty toomen_public_keyin theDeployedOmenAgentclass and its usage in theget_resolved_betsmethod improves clarity and consistency with the terminology used in the Omen prediction market. This change makes the code more understandable and maintainable.- 30-42: The addition of the
from_api_keysstatic method to initializeDeployedOmenAgentinstances with API keys is a good practice. It encapsulates the initialization logic, making the code more modular and easier to maintain.- 43-57: The
from_monitor_settingsstatic method provides a clear and concise way to create instances ofDeployedOmenAgentbased on monitoring settings. This method enhances the modularity and maintainability of the code by abstracting the instantiation logic.- 58-66: The
from_all_gcp_functionsclass method is a valuable addition for retrieving all deployed agents on GCP for specific market types. This method supports the objective of streamlining the management and oversight of deployed agents, contributing to the overall maintainability and scalability of the monitoring logic.prediction_market_agent_tooling/markets/manifold/api.py (1)
- 69-69: Using
get_secret_value()to access themanifold_api_keyin theplace_betfunction is a secure practice. This approach ensures that the API key is handled securely, minimizing the risk of accidental exposure.prediction_market_agent_tooling/deploy/agent.py (5)
- 6-15: The addition of imports for
datetime,git,APIKeys, and constants likeMARKET_TYPE_KEY,REPOSITORY_KEY, andCOMMIT_KEYsupports the enhanced deployment and monitoring capabilities introduced in this PR. These changes are necessary for the new functionalities and improve the code's maintainability by clearly separating concerns.- 77-84: Modifying the
deploy_gcpfunction to includeapi_keysandstart_timeparameters is a significant improvement. It allows for more granular control over the deployment process and supports the introduction of monitoring capabilities. This change enhances the flexibility and functionality of the deployment process.- 115-120: The logic to instantiate
monitor_agentbased onmarket_type,start_time, andapi_keysis a crucial addition. It aligns with the PR's objectives to enhance monitoring capabilities. This approach improves the modularity and maintainability of the code by encapsulating the instantiation logic within a dedicated method.- 102-109: The enhancement of environment variable setup with monitor configuration data, including the use of labels and environment variables for repository and commit information, is a good practice. It improves the clarity and maintainability of the deployment process, ensuring that crucial information is readily available and correctly managed.
- 103-103: Given the previous comments regarding the distinction between labels and environment variables, it's clear that the decision to use environment variables for detailed configuration and labels for high-level categorization is a thoughtful approach. This strategy enhances the usability and flexibility of the deployment process.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- prediction_market_agent_tooling/config.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- prediction_market_agent_tooling/config.py
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- prediction_market_agent_tooling/deploy/gcp/utils.py (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- prediction_market_agent_tooling/deploy/gcp/utils.py
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- prediction_market_agent_tooling/deploy/agent.py (4 hunks)
- prediction_market_agent_tooling/monitor/monitor.py (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- prediction_market_agent_tooling/deploy/agent.py
- prediction_market_agent_tooling/monitor/monitor.py
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- prediction_market_agent_tooling/markets/manifold/manifold.py (1 hunks)
Additional comments: 1
prediction_market_agent_tooling/markets/manifold/manifold.py (1)
- 47-47: The update to use
model.probabilityfor thep_yesparameter in thefrom_data_modelmethod aligns with the PR objectives to refine how market probabilities are handled. This change seems logical and correct, assumingmodel.probabilityprovides the intended probability value for the "yes" outcome directly, which simplifies the process compared to extracting it frommodel.pool.YES.Ensure that all downstream code that interacts with the
p_yesproperty ofManifoldAgentMarketinstances is aware of this change and correctly interprets the probability values. This might include any logic that makes decisions based on these probabilities or any display logic that presents these values to users.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- prediction_market_agent_tooling/monitor/monitor.py (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- prediction_market_agent_tooling/monitor/monitor.py
Summary by CodeRabbit