-
Notifications
You must be signed in to change notification settings - Fork 3
feat: Support streaming #57
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
base: main
Are you sure you want to change the base?
Conversation
6e38f89
to
a73be0f
Compare
Signed-off-by: Jeff Napper <[email protected]>
Signed-off-by: Jeff Napper <[email protected]>
a73be0f
to
33ce664
Compare
Signed-off-by: Jeff Napper <[email protected]>
Signed-off-by: Jeff Napper <[email protected]>
Signed-off-by: Jeff Napper <[email protected]>
Signed-off-by: Jeff Napper <[email protected]>
def parse_args() -> argparse.Namespace: | ||
parser = argparse.ArgumentParser(description="Agent Workflow Server") | ||
parser.add_argument("--host", default=os.getenv("API_HOST", DEFAULT_HOST)) | ||
parser.add_argument( | ||
"--port", type=int, default=int(os.getenv("API_PORT", DEFAULT_PORT)) | ||
) | ||
parser.add_argument( | ||
"--num-workers", type=int, default=int(os.environ.get("NUM_WORKERS", 5)) | ||
) | ||
parser.add_argument( | ||
"--agent-manifest-path", | ||
action="append", | ||
type=pathlib.Path, | ||
default=[os.getenv("AGENT_MANIFEST_PATH", "manifest.json")], | ||
) | ||
parser.add_argument("--agents-ref", default=os.getenv("AGENTS_REF", None)) | ||
parser.add_argument( | ||
"--log-level", default=os.environ.get("NUM_WORKERS", logging.INFO) | ||
) | ||
return parser.parse_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.
Any specific reason why we added CLI arguments as well? (alongside env vars)
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 was for developer experience so that a help could be generated and users can run the server without hidden settings. Env vars are not always obvious to users.
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.
The workflow server (as stated in the docs) is not user-facing. The user-facing part is workflow server manager. For a developer, we can improve documentation/readme. Also I don't see how the current options would guide the user (I don't see any description for the options here). Last but not least, it's more code to maintain (and happy not to do it, if it's not really really needed).
Long story short, I do suggest to remove CLI args here. If env vars - which most of them are optional - are not self explicatory we can improve documentation.
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.
Overall lgtm.
Alongside some minor fixes/comments, unit tests for the new stream_events
function are missing.
Also, I see this only supports values
stream mode, and not custom
. Is this intentional?
Signed-off-by: Jeff Napper <[email protected]>
Yes, and yes. The values mode was tested alongside the client in the interest of time. We will need to add the unit tests and custom support later. I did not have a test agent that sent custom stream events yet. |
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.
Based on the comments above:
- Remove CLI args (not needed imho)
- This PR adds support for
values
stream mode only. StreamMode type is included in RunCreate(Stateless) requests. This should be checked, so in case stream_mode == custom we should trigger a NotImplemented error
Description
This PR adds support for SSE streaming of agent value stream events.
Type of Change
Checklist