-
Notifications
You must be signed in to change notification settings - Fork 19
Refactor service spawning: add ForgeActor.options().as_service() API #153
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
Merged
Merged
Changes from 15 commits
Commits
Show all changes
34 commits
Select commit
Hold shift + click to select a range
da21e1d
Add reward interface, math reward, unit tests
DNXie 5c72908
Merge branch 'meta-pytorch:main' into main
DNXie b4d7a61
Merge branch 'meta-pytorch:main' into main
DNXie 02d77c6
Merge branch 'meta-pytorch:main' into main
DNXie fd1d38b
Merge branch 'meta-pytorch:main' into main
DNXie f79beee
Merge branch 'meta-pytorch:main' into main
DNXie d8d775a
Merge branch 'meta-pytorch:main' into main
DNXie e423c44
Merge branch 'meta-pytorch:main' into main
DNXie 4815c05
Merge branch 'meta-pytorch:main' into main
DNXie 77d41e4
Merge branch 'meta-pytorch:main' into main
DNXie a3feb1e
Merge branch 'meta-pytorch:main' into main
DNXie 23d7e02
Merge branch 'meta-pytorch:main' into main
DNXie 2ca881d
refactor, buggy
DNXie 4df5d3a
some tweak
DNXie 0b5c0db
add options under forgeactor, and tested it with vllm main
DNXie 1cc5cf2
add as_service to forgeactor for default config
DNXie a92952a
enable passing serviceConfig obj to options
DNXie 2ce61d1
update all the usages
DNXie f32fef7
Merge branch 'main' into add_options
DNXie f28824d
fix script error and CI broken test
DNXie 549f43a
fix ci test
DNXie 26a4207
remove redundant line
DNXie a311cbd
Update tests/unit_tests/test_service.py
DNXie 1261568
Merge branch 'main' into add_options
DNXie c1854ec
add missing import back
DNXie e595fbd
fix ci
DNXie 52cb676
Merge branch 'add_options' of github.com:DNXie/forge into DNXie-add_o…
DNXie fd38100
Merge branch 'DNXie-add_options'
DNXie 9a80b16
Merge branch 'main' of github.com:DNXie/forge
DNXie 09e7237
merge
DNXie 0165027
make options return a forgeactor and as_service return a serviceinter…
DNXie 721e32a
remove redundant test case
DNXie 915baf1
fix lint
DNXie 4171675
fix broken ci
DNXie File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 remove the
ConfiguredService
piece altogether?options()
should return atype["ForgeActor"]
Uh oh!
There was an error while loading. Please reload this page.
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 main reason we have the
ConfiguredService
wrapper is to allow direct access to actor endpoints via instance attributes (e.g., service.value.choose()) while still carrying theServiceConfig
on the class.If we remove the wrapper and make
options()
return a subclass ofForgeActor
directly, the endpoints (likecounter.value
) are only accessible through the underlying ServiceInterface. So we need to change the statementto
I attempted to delegate
EndpointProperty
access via__getattr__
like thisHowever, it didn’t fully work: Python descriptors like
EndpointProperty
only bind correctly when accessed through the class or a properly initialized ServiceInterface instance. The__get__
call in__getattr__
does not fully replicate the descriptor binding behavior.I’m still getting familiar with the internals of Monarch and some of the Python descriptor mechanics, so I'd love to hear any suggestions if there’s a cleaner way to handle this.
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.
hmm,
EndpointProperty
binding is only needed after we doas_service()
right?this is how I imagine the
options
piece:Uh oh!
There was an error while loading. Please reload this page.
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.
Yes. It is needed whenever we call an endpoint function.
Your proposed implementation seems reasonable. However, it does not handle endpoint binding. After
as_service()
, you still need to go throughservice._service_interface
to access endpoints.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.
As we discussed offline, if
as_service
returns aServiceInterface
directly, we cannot terminate the service withbecause the returned object (
service
) is just aServiceInterface
. In that case, we’d have to fall back toPersonally, I prefer the
service.shutdown()
style since it feels more natural and object-oriented, but I’m okay with either one.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 can add
def shutdown(self)
directly toServiceInterface
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.
Done!