-
Notifications
You must be signed in to change notification settings - Fork 2
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
Only one instance on GCP deployment and small tweaks #18
Conversation
WalkthroughThe recent updates introduce a more flexible deployment strategy for agents on Google Cloud Platform (GCP), enhancing the deployment script to support dynamic agent selection, scheduling, and branch configuration. Additionally, the prediction market agent tooling has been improved with new agent behaviors, including a random market selection and a deliberate error-raising agent. Furthermore, deployment utility enhancements include timeout settings and automatic retry on failure, improving reliability and control over deployment processes. Changes
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 (
|
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.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (3)
- examples/cloud_deployment/gcp/deploy.py (2 hunks)
- prediction_market_agent_tooling/deploy/agent_example.py (1 hunks)
- prediction_market_agent_tooling/deploy/gcp/utils.py (2 hunks)
Additional comments: 4
prediction_market_agent_tooling/deploy/agent_example.py (2)
- 9-9: The change to
DeployableCoinFlipAgent
simplifies the market selection process by always returning a random sample of markets. This approach assumes there is at least one market available. Ensure that the list of markets passed topick_markets
is never empty to avoid potential runtime errors.- 15-17: The introduction of
DeployableAlwaysRaiseAgent
with a method that unconditionally raises aRuntimeError
is a clear way to test error handling mechanisms. However, ensure that this behavior aligns with the testing strategy and that there are mechanisms in place to catch and properly handle this exception during deployment tests.examples/cloud_deployment/gcp/deploy.py (1)
- 36-37: The use of
typer.run(main)
to execute the script provides a simple and effective CLI interface. Ensure that the documentation is updated to reflect the new parameters and usage instructions for deploying agents.prediction_market_agent_tooling/deploy/gcp/utils.py (1)
- 18-19: Adding
timeout
andretry_on_failure
parameters to thegcloud_deploy_cmd
function improves the configurability of deployment operations. Ensure that the default values chosen for these parameters are sensible for the majority of deployment scenarios. Additionally, document these parameters to inform users about how to use them effectively.
def main( | ||
agent_name: str, cron_schedule: str = "0 */2 * * *", branch: str = "main" | ||
) -> None: | ||
agent: DeployableAgent = { | ||
"coin_flip": DeployableCoinFlipAgent, | ||
"always_raise": DeployableAlwaysRaiseAgent, | ||
}[agent_name]() |
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.
The dynamic selection of agents based on the agent_name
parameter is a significant improvement for deployment flexibility. However, consider validating the agent_name
input against the available agent classes to prevent runtime errors from invalid inputs.
+ if agent_name not in ["coin_flip", "always_raise"]:
+ raise ValueError(f"Invalid agent_name: {agent_name}. Available options: 'coin_flip', 'always_raise'")
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.
def main( | |
agent_name: str, cron_schedule: str = "0 */2 * * *", branch: str = "main" | |
) -> None: | |
agent: DeployableAgent = { | |
"coin_flip": DeployableCoinFlipAgent, | |
"always_raise": DeployableAlwaysRaiseAgent, | |
}[agent_name]() | |
def main( | |
agent_name: str, cron_schedule: str = "0 */2 * * *", branch: str = "main" | |
) -> None: | |
if agent_name not in ["coin_flip", "always_raise"]: | |
raise ValueError(f"Invalid agent_name: {agent_name}. Available options: 'coin_flip', 'always_raise'") | |
agent: DeployableAgent = { | |
"coin_flip": DeployableCoinFlipAgent, | |
"always_raise": DeployableAlwaysRaiseAgent, | |
}[agent_name]() |
f"--timeout {timeout}s " | ||
# Explicitly set no concurrency, min instances to 0 (agent is executed only once in a while) and max instances to 1 (parallel agents aren't allowed). | ||
"--concurrency 1 " | ||
"--min-instances 0 " | ||
"--max-instances 1 " | ||
) | ||
if retry_on_failure: | ||
cmd += "--retry " |
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.
The implementation of the timeout
and retry_on_failure
parameters in the deployment command is correct. However, consider adding error handling or logging to provide feedback when a deployment fails despite retries. This can help in diagnosing deployment issues more efficiently.
+ if retry_on_failure:
+ cmd += "--retry "
+ print("Note: Deployment will retry on failure.")
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.
f"--timeout {timeout}s " | |
# Explicitly set no concurrency, min instances to 0 (agent is executed only once in a while) and max instances to 1 (parallel agents aren't allowed). | |
"--concurrency 1 " | |
"--min-instances 0 " | |
"--max-instances 1 " | |
) | |
if retry_on_failure: | |
cmd += "--retry " | |
f"--timeout {timeout}s " | |
# Explicitly set no concurrency, min instances to 0 (agent is executed only once in a while) and max instances to 1 (parallel agents aren't allowed). | |
"--concurrency 1 " | |
"--min-instances 0 " | |
"--max-instances 1 " | |
) | |
if retry_on_failure: | |
cmd += "--retry " | |
print("Note: Deployment will retry on failure.") |
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- examples/cloud_deployment/gcp/deploy.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- examples/cloud_deployment/gcp/deploy.py
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- examples/cloud_deployment/gcp/deploy.py (2 hunks)
- prediction_market_agent_tooling/markets/data_models.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- examples/cloud_deployment/gcp/deploy.py
Additional comments: 1
prediction_market_agent_tooling/markets/data_models.py (1)
- 159-159: The change to make
lastBetTime
optional in theManifoldMarket
class by usingt.Optional[datetime]
is appropriate for scenarios where the last bet time might not be available. This enhances the flexibility of the data model to represent various states of a market.Ensure that all parts of the codebase that interact with
ManifoldMarket
instances are updated to handleNone
values forlastBetTime
gracefully. This might involve adding checks or default values wherelastBetTime
is used to prevent any runtime errors or unexpected behavior.
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- examples/cloud_deployment/gcp/deploy.py (2 hunks)
- prediction_market_agent_tooling/deploy/agent.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- examples/cloud_deployment/gcp/deploy.py
Additional comments: 3
prediction_market_agent_tooling/deploy/agent.py (3)
- 74-74: The addition of the
gcp_fname
parameter with a default value ofNone
to thedeploy_gcp
method is a good practice for enhancing flexibility. This allows for dynamic naming of GCP functions based on the deployment context, which aligns with the PR's objectives of improving the deployment process's configurability and reliability.- 89-89: The logic to assign
gcp_fname
using a conditional expression is correctly implemented. It ensures that ifgcp_fname
is provided, it will be used; otherwise, it falls back to generating a name using theget_gcloud_fname
method. This approach maintains backward compatibility while introducing new functionality, which is a positive aspect of these changes.- 71-77: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-104]
Overall, the
DeployableAgent
class and the changes made in thedeploy_gcp
method align well with the PR's objectives of enhancing the deployment process to GCP. The code is well-structured, follows Python best practices, and introduces meaningful improvements to the deployment process's flexibility and reliability. It's recommended to continue monitoring the deployment process for any edge cases or errors that may arise from the new changes and to consider adding unit tests for the new functionality to ensure its correctness and stability.
return "Success" | ||
""" | ||
|
||
gcp_fname = self.get_gcloud_fname(market_type) | ||
gcp_fname = gcp_fname or self.get_gcloud_fname(market_type) | ||
|
||
with tempfile.NamedTemporaryFile(mode="w", suffix=".py") as f: | ||
f.write(entrypoint_template) |
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.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [74-104]
The deploy_gcp
method demonstrates a comprehensive approach to deploying agents to GCP, including dynamic entrypoint generation, deployment parameter handling, and error checking. However, it's important to ensure that the deploy_to_gcp
and related functions are robustly implemented to handle the dynamic nature of the entrypoint generation and deployment parameters. Additionally, consider adding more detailed error messages to help diagnose deployment failures more effectively.
Consider enhancing error messages in the deployment process to provide more context on failures, which could aid in troubleshooting.
) | ||
if retry_on_failure: | ||
cmd += "--retry " |
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 not sure what happened before with OpenAI-powered agents, this retry
is disabled by default. I tried to deploy agents that always fail with changes from this PR and without, and both are executed just once per 10 min (as scheduled).
I think this PR introduces changes that are worth merging, but it doesn't solve the retries problem. I'll deploy the openai-powered agents tomorrow and monitor it...
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.
LGTM
Summary by CodeRabbit
DeployableAlwaysRaiseAgent
class that raises aRuntimeError
with a specific message for binary market scenarios.DeployableCoinFlipAgent
to consistently return a random sample of markets.timeout
andretry_on_failure
parameters for more reliable deployments.