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
Refactoring deployment a bit #15
Refactoring deployment a bit #15
Changes from all commits
3097f12
1c15bc3
fc0eb7e
f0ded87
0e5f10f
56fe592
8a659a5
2e1d416
11967d1
72248a1
21a2d11
edab35b
0c3ad47
44fc180
122de0a
1a15c43
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
much cleaner :). Removing
remove_deployed_gcp_function
though - living a high-risk lifestyle!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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 will return path to the agent's implementation, not to this abstract class.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wooo nice. The problem I saw with not making
run
a static function is that the user can now define their agent like: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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But even if
run
is a static function, isn't the problem still the same? Even ifrun
is static, they are still able to doso, nothing changes, or does it?
Doing
{self.__class__.__name__}()
assumes that the class doesn't take any init arguments, but ifrun
is static, the class needs to be initialised somewhere, so it's the same issue.What we could do is
and use
{self.__class__.__name__}.create_agent()
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 would say that because the static method can't access
self
, it doesn't depend onFoo
to run 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.
Just tried with this branch, and e.g.
which produced
(the
<some path>
string was a bit funky, but ignoring for now). This failed with: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, I'd expect it to fail, as
foo
is the required argument in yourFooAgent
.So you could do
And instead of calling
FooAgent()
in the generated function, we would callFooAgent.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:
Now we don't have
self
anymore, so we need to change it, but how? We can't dobecause that way, anything subclassed won't be used, the parent abstract class will always be used.
And we also can't do
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 ascreate_agent
I proposed above anyway.Or did you have something else in mind?
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.
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:
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.
Wau I didn't know about
__init_subclass__
:o Added!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.
Changed to a normal method with self, fixed the name in gcp:
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.
Could just make them default empty dicts to save a few chars below on if statements
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.
Isn't defaulting to dicts/lists/... forbidden pattern in Python, because of mutability issues?
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 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!