-
Notifications
You must be signed in to change notification settings - Fork 188
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
feat(tasks): accept args in BaseTask.run()
#1598
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
335f6ec
to
f2711cd
Compare
f2711cd
to
c7bd1a6
Compare
@collindutter what else needs to be done here? |
I had some last minute ideas I wanted to try out. Will implement soon to see how they feel. |
cdf5272
to
1a5ef91
Compare
BaseTask.run()
@vachillo I updated/simplified the PR if you'd like to take one more look. |
1a5ef91
to
5772e2e
Compare
|
@vachillo how does updated version look? |
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 find it kind of confusing that there is two entrypoints into the prompt via args
and context
. what would be the advantage of either way?
griptape/structures/structure.py
Outdated
@@ -197,7 +196,8 @@ def after_run(self) -> None: | |||
def add_task(self, task: BaseTask) -> BaseTask: ... | |||
|
|||
@observable | |||
def run(self, *args) -> Structure: | |||
def run(self, *args, **kwargs) -> Structure: |
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.
lets remove **kwargs
here
griptape/structures/structure.py
Outdated
@@ -154,7 +154,6 @@ def resolve_relationships(self) -> None: | |||
@observable | |||
def before_run(self, args: Any) -> None: | |||
super().before_run(args) | |||
self._execution_args = args |
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.
how does the move to .run
for this line going to affect 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.
Reverted it
_execution_args: tuple = field(factory=tuple, init=False) | ||
|
||
@property | ||
def execution_args(self) -> tuple: | ||
return self._execution_args |
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 these be serializable?
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 don't think so, they are not intended to be static, serializable data.
5772e2e
to
b204e62
Compare
For example: task = PromptTask(
input="You are talking to {{ user_name }} ({{ user_id }}).They just said: {{ args[0] }}",
context={
"user_name": "John Doe",
"user_id": "123",
},
)
task.run("Hello, how are you?")
task.run("How do I foo the bar?") |
i see. i think we just have very different ways of thinking about how this stuff gets run. to me, its unrealistic that you would invoke |
It would be preferable if you didn't need to create a new Task every time though, as there is some overhead associated with creating it. With the upcoming async work I think that model becomes more feasible since you can run the async work on the main thread vs creating a new one. |
Describe your changes
BaseTask.run()
now acceptsargs
just likeStructure.run()
. Decided against adding kwargs support in this PR since there is a future opportunity for special kwargs likecontext=
. Having a blackholekwargs
that is dumped into context makes it challenging to add kwargs that shouldn't go in context.Issue ticket number and link
Closes #1597
📚 Documentation preview 📚: https://griptape--1598.org.readthedocs.build//1598/