-
Notifications
You must be signed in to change notification settings - Fork 65
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
removing nested-asyncio from Model Registry client #802
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
2ef490c
to
ec997c6
Compare
@@ -0,0 +1,102 @@ | |||
# from https://gist.github.com/blink1073/969aeba85f32c285235750626f2eadd8 |
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 should verify if alternative libraries exists on pypi to avoid having to introduce multiple licensing in the project itseld
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.
@tarilabs, We had the same need in our internal project, and this was the only solution that I found after more than a week of searches and conversations with my colleagues. If you can find a better solution for this I will be very interested to hear it
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.
Sure, but maybe there are ways we could allow user to plug their own executor/runner strategies, without requiring necessarily to opt for one or the other ourselves, maybe picking one
(this is because seems to me at the end of the day it's the same signature/contract)
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.
Do you mean introducing a base class and using AsyncTaskRunner as a default? To make ModelRegistry independent of it we will also need a factory class, doing instantiation.
So add 2 additional classes. So yes, this can be done. Is it worth it?
If the answer is yes, we can do it
Signed-off-by: blublinsky <[email protected]>
Signed-off-by: blublinsky <[email protected]>
775f13a
to
6bd81c8
Compare
Description
Replaced async-runner by a new implementation to avoid usage of nested-async to address #774
How Has This Been Tested?
Unit tests and running in our Ray environment
Merge criteria:
DCO
check)If you have UI changes