-
Notifications
You must be signed in to change notification settings - Fork 16
Leverage Services for Policy + Rename PolicyRouter #70
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
Conversation
Code-wise this is pretty close to what we will be using Debugging an issue where |
src/forge/actors/policy.py
Outdated
class Policy(PolicyInterface): | ||
# TODO: Add dp support | ||
policy: Actor | ||
config: DictConfig |
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 like individual components having knowledge of our frontend config system. Can we parameterize this fully or pass in a policy specific config 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.
looking good!
await router.shutdown.call() | ||
|
||
|
||
if __name__ == "__main__": |
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 move the policy main into its own app? apps/vllm.py or something
I think having a standalone vLLM application will be useful for Stanford too
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.
Totally let's do it in a. separate PR to unblock this one
lgtm, as long as it's running! |
Recreation of #61, since commit history got borked
Leverage
spawn_services
for Policy and performs associated refactorsPolicyRouter
->Policy
Policy
->PolicyWorker