-
Notifications
You must be signed in to change notification settings - Fork 21
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
Step Metadata #26
Step Metadata #26
Conversation
tests are still red, working on it |
tapeagents/core.py
Outdated
|
||
def llm_dict(self) -> dict: | ||
return self.model_dump(exclude={"prompt_id", "task", "by"}, exclude_none=True) | ||
def task(self, task: str) -> Self: |
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 this become node?
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.
Probably, I want to discuss that with Dima one more time to make things more clear in my head :)
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 see that this method mutates the object, so at the very least I'd call it set_task
, but @jpt-sn 's comment is more important.
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 .by_node()
. Are you ok with that 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.
Looks almost good! Please set the node that made the step in tapeagents.core.agent
.
Co-authored-by: Dzmitry Bahdanau <[email protected]>
it's amazing to see it done!!! thanks, @ollmer! |
move prompt id, task and by to the step metadata