-
Notifications
You must be signed in to change notification settings - Fork 7
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
Added a tool server that exposes all tools as endpoints #79
Conversation
Co-authored-by: James Braza <jamesbraza@gmail.com>
class EnvStateMessage(Message): | ||
"""A message that contains the current state of the environment.""" |
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.
Is this used at all?
And if it's used, do you mind documenting where the current state gets housed (e.g. is it JSON in the content
)?
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 is related to tree search - I had to promote this code to prevent dependence on aviary-internal
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.
So, I think when @sidnarayanan promotes the rest of his code it will be documented then.
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.
Fwiw, tree search is not going to be in aviary, it will be in LDP. Should we put it in LDP with tree search?
I guess I think it's confusing to have this message subclass here without usage or docs, and no specialized behaviors
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.
EnvStateMessage
should be in aviary
- it can be used by environments to indicate that a particular message in the returned observation represents the state of the env. I don't see any harm in keeping it here.
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.
We'll have to work this out in another PR - basically @sidnarayanan needs this message type in cloning, cloning should not depend on ldp or aviary-internal.
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.
Can we at least put Sid's comment into the docstring for EnvStateMessage
then?
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.
Beyond "A message that contains the current state of the environment." ?
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 about something like this:
class EnvStateMessage(Message): | |
"""A message that contains the current state of the environment.""" | |
class EnvStateMessage(Message): | |
""" | |
A message variant whose contents are known to contain the environment state. | |
For example, the contents can be a JSON serialization of the environment state. | |
""" |
@pytest.mark.asyncio | ||
async def test_make_tool_server(): | ||
def add(a: int, b: int) -> int: | ||
"""Add two numbers.""" |
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.
"""Add two numbers.""" | |
"""Add two numbers.""" # Allows testing empty descriptions |
This adds a new method -
make_tool_server
- that will create an authenticated server that has all environment tools exposed as endpoints.This is used in a new entry point:
which will then start a server and host the tools: