Refactoring deployment a bit #15
Conversation
WalkthroughThe recent updates focus on refining deployment processes for both local environments and Google Cloud Platform (GCP). Key enhancements include the introduction of 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 (
|
kongzii
force-pushed
the
peter/gcp-secrets-labels
branch
from
0079c55
to
56fe592
Compare
![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: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
mypy.iniis excluded by:!**/*.inipoetry.lockis excluded by:!**/*.lock
Files selected for processing (6)
- examples/cloud_deployment/gcp/deploy.py (2 hunks)
- prediction_market_agent_tooling/deploy/agent.py (4 hunks)
- prediction_market_agent_tooling/deploy/agent_example.py (2 hunks)
- prediction_market_agent_tooling/deploy/gcp/deploy.py (3 hunks)
- prediction_market_agent_tooling/deploy/gcp/utils.py (2 hunks)
- tests/deploy/test_deploy.py (2 hunks)
Additional comments: 6
prediction_market_agent_tooling/deploy/agent_example.py (1)
- 1-7: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-11]
Changes align with PR objectives to streamline and simplify the deployment process by removing unnecessary imports and the Flask-based HTTP entry point. The refactoring enhances modularity and maintainability.
tests/deploy/test_deploy.py (1)
- 1-6: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [3-19]
The change from
deploytodeploy_localand the removal of thedeployment_typeparameter align with the PR objectives, contributing to a more modular and clear deployment process.examples/cloud_deployment/gcp/deploy.py (1)
- 1-13: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [3-21]
The use of
DeployableCoinFlipAgentand thedeploy_gcpmethod with updated parameters like memory and cron schedule aligns with the PR objectives, improving the deployment process's modularity and efficiency.prediction_market_agent_tooling/deploy/gcp/deploy.py (1)
- 23-29: The addition of the
gcp_fnameparameter and the update to allowNonetypes forlabels,env_vars, andsecretsenhance the function's flexibility and align with the PR objectives, contributing to a more flexible and refined deployment process.prediction_market_agent_tooling/deploy/gcp/utils.py (1)
- 27-41: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [14-38]
Making
labels,env_vars, andsecretsoptional and adding checks to ensure they are notNonebefore use simplifies the deployment command building process and aligns with the PR objectives, contributing to a more streamlined and simplified deployment process.prediction_market_agent_tooling/deploy/agent.py (1)
- 35-106: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-122]
The introduction of
deploy_localanddeploy_gcpmethods, along with the additional logic for GCP deployment, aligns with the PR objectives, significantly improving the modularity and efficiency of the deployment process.
| def main(request) -> str: | ||
| {self.__class__.__name__}().run(market_type={market_type.__class__.__name__}.{market_type.name}) | ||
| return "Success" | ||
| """ |
There was a problem hiding this comment.
I'm not happy with this, but it works for now. We can iterate again later.
During the implementation, I somehow didn't see how static functions would help in the end. I wanted to use them but then ended up with the current solution.
Please comment if I missed something.
There was a problem hiding this comment.
Wooo nice. The problem I saw with not making run a static function is that the user can now define their agent like:
from pydantic import BaseModel
from prediction_market_agent_tooling.deploy.agent import DeployableAgent
class Foo(BaseModel):
x: int
class DeployableCoinFlipAgent(DeployableAgent):
foo: Foo
...
so now can have arbitrary dependencies that we need to know about to initialize the agent. e.g. in this example we need to import and create an instance of Foo, so deployment will fail.
There was a problem hiding this comment.
But even if run is a static function, isn't the problem still the same? Even if run is static, they are still able to do
class DeployableCoinFlipAgent(DeployableAgent):
foo: Fooso, nothing changes, or does it?
Doing{self.__class__.__name__}() assumes that the class doesn't take any init arguments, but if run is static, the class needs to be initialised somewhere, so it's the same issue.
What we could do is
class DeployableAgent:
@staticmethod
def create_agent() -> "DeployableAgent":
"""Override this method if your agents needs custom initialization"""
return DeployableAgent()and use {self.__class__.__name__}.create_agent()
There was a problem hiding this comment.
I would say that because the static method can't access self, it doesn't depend on Foo to run it.
There was a problem hiding this comment.
Just tried with this branch, and e.g.
from pydantic import BaseModel
from prediction_market_agent_tooling.deploy.agent import DeployableAgent
from prediction_market_agent_tooling.markets.data_models import AgentMarket
from prediction_market_agent_tooling.markets.markets import MarketType
class Foo(BaseModel):
answer: bool
class FooAgent(DeployableAgent):
def __init__(self, foo: Foo):
self.foo = foo
def answer_binary_market(self, market: AgentMarket) -> bool:
return self.foo.answer
if __name__ == "__main__":
agent = FooAgent(foo=Foo(answer=True))
agent.deploy_gcp(
repository="git+https://github.com/gnosis/prediction-market-agent-tooling.git@peter/refactor-deployment",
market_type=MarketType.MANIFOLD,
memory=1024,
)
which produced
import functions_framework
from <some path> import *
from prediction_market_agent_tooling.markets.markets import MarketType
@functions_framework.http
def main(request) -> str:
FooAgent().run(market_type=MarketType.MANIFOLD)
return "Success"
(the <some path> string was a bit funky, but ignoring for now). This failed with:
FooAgent().run(market_type=MarketType.MANIFOLD)
TypeError: FooAgent.__init__() missing 1 required positional argument: 'foo'
There was a problem hiding this comment.
Yeah, I'd expect it to fail, as foo is the required argument in your FooAgent.
So you could do
class FooAgent(DeployableAgent):
def __init__(self, foo: Foo):
self.foo = foo
def answer_binary_market(self, market: AgentMarket) -> bool:
return self.foo.answer
@staticmethod
def create_agent() -> "FooAgent":
return FooAgent(foo=...)And instead of calling FooAgent() in the generated function, we would call FooAgent.create_agent().
I think our situation is very similar to the MLFlow, https://mlflow.org/docs/latest/_modules/mlflow/pyfunc/model.html#PythonModel, they also have some "parent class" that needs to be subclassed, and then this subclass can be deployed by MLFlow-tools without any worries. They don't allow to change init as well, instead there are special methods that are auto-called in the deployment.
But back to the staticmethod, let's say we have this:
class DeployableAgent:
...
@staticmethod
def run(market_type: MarketType, _place_bet: bool = True) -> None:
available_markets = [
x.to_agent_market() for x in get_binary_markets(market_type)
]
markets = self.pick_markets(available_markets)
for market in markets:
result = self.answer_binary_market(market)
if _place_bet:
print(f"Placing bet on {market} with result {result}")
place_bet(
market=market.original_market,
amount=get_tiny_bet(market_type),
outcome=result,
omen_auto_deposit=True,
)Now we don't have self anymore, so we need to change it, but how? We can't do
result = DeployableAgent.answer_binary_market(market)
because that way, anything subclassed won't be used, the parent abstract class will always be used.
And we also can't do
@staticmethod
def run(agent: DeployableAgent, market_type: MarketType, _place_bet: bool = True) -> None:
...
because we would be again at the square 0, we would need to initialise the agent inside of the generated main function, and that initialisation could fail, so we would need something as create_agent I proposed above anyway.
Or did you have something else in mind?
There was a problem hiding this comment.
Ah you're right! Happy to go with this option then. We can add a guard to the base class to throw an exception if the derived class tries to override the init method:
class Base:
def __init__(self):
pass
def __init_subclass__(cls, **kwargs):
if cls.__init__ is not Base.__init__:
raise TypeError("Cannot override __init__ method of Base class")
class Derived(Base):
# Raises a TypeError
def __init__(self):
pass
There was a problem hiding this comment.
Wau I didn't know about __init_subclass__ :o Added!
| secrets: dict[str, str] | None = None, | ||
| cron_schedule: str | None = None, | ||
| ) -> None: | ||
| path_to_agent_file = os.path.relpath(inspect.getfile(self.__class__)) |
There was a problem hiding this comment.
This will return path to the agent's implementation, not to this abstract class.
| def get_gcloud_fname(cls, market_type: MarketType) -> str: | ||
| return f"{cls.__class__.__name__.lower()}-{market_type}-{int(time.time())}" | ||
| def get_gcloud_fname(self, market_type: MarketType) -> str: | ||
| return f"{self.__class__.__name__.lower()}-{market_type}-{int(time.time())}" |
There was a problem hiding this comment.
Changed to a normal method with self, fixed the name in gcp:
| def main(request) -> str: | ||
| {self.__class__.__name__}().run(market_type={market_type.__class__.__name__}.{market_type.name}) | ||
| return "Success" | ||
| """ |
There was a problem hiding this comment.
Wooo nice. The problem I saw with not making run a static function is that the user can now define their agent like:
from pydantic import BaseModel
from prediction_market_agent_tooling.deploy.agent import DeployableAgent
class Foo(BaseModel):
x: int
class DeployableCoinFlipAgent(DeployableAgent):
foo: Foo
...
so now can have arbitrary dependencies that we need to know about to initialize the agent. e.g. in this example we need to import and create an instance of Foo, so deployment will fail.
| @@ -27,18 +17,6 @@ | |||
| secrets={ | |||
| "MANIFOLD_API_KEY": f"JUNG_PERSONAL_GMAIL_MANIFOLD_API_KEY:latest" | |||
| }, # Must be in the format "env_var_in_container => secret_name:version", you can create secrets using `gcloud secrets create --labels owner=<your-name> <secret-name>` command. | |||
| memory=512, | |||
| memory=256, | |||
| cron_schedule="0 */2 * * *", | |||
There was a problem hiding this comment.
much cleaner :). Removing remove_deployed_gcp_function though - living a high-risk lifestyle!
There was a problem hiding this comment.
Should it be auto-removed after it's scheduled to run every 2 hours? I thought that was there only for the example purposes.
There was a problem hiding this comment.
I guess the example was there originally as something that demos all the deployment functionality but destroys everything at the end, kind of like a test that the user could run. Just guarding against the user running it without realising that they're spawning a forever-alive agent! No big deal though
| labels: dict[str, str], | ||
| env_vars: dict[str, str], | ||
| secrets: dict[str, str], | ||
| labels: dict[str, str] | None, |
There was a problem hiding this comment.
Could just make them default empty dicts to save a few chars below on if statements
There was a problem hiding this comment.
Isn't defaulting to dicts/lists/... forbidden pattern in Python, because of mutability issues?
There was a problem hiding this comment.
😱 I wasn't aware of that before! TIL...
Explained well here: https://chat.openai.com/share/045afe93-640a-43e7-91b7-24c7e32a197e
Not an issue in this case I guess, as these dicts are only read, not written to. But thanks for the lesson and I retract my suggestion!
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/agent.py (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- prediction_market_agent_tooling/deploy/agent.py
Summary by CodeRabbit
Noneforlabels,env_vars, andsecretsin GCP deployment functions.