-
Notifications
You must be signed in to change notification settings - Fork 144
add options to create_scheduler so that the get_runner method is full… #767
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
c3eeca5
to
ac577c0
Compare
…y configurable I also added a note to each schedulers __init__ method to help with maintainablility Signed-off-by: Kevin <kpostlet@redhat.com>
ac577c0
to
35218e7
Compare
Codecov Report
@@ Coverage Diff @@
## main #767 +/- ##
=======================================
Coverage 92.88% 92.88%
=======================================
Files 96 96
Lines 6096 6098 +2
=======================================
+ Hits 5662 5664 +2
Misses 434 434
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
LGTM -- thanks! A unit test would be nice but not critical and fixing lint
) | ||
|
||
return RayScheduler(session_name=session_name) | ||
return RayScheduler(session_name=session_name, ray_client=ray_client) |
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.
would be nice to add some unit tests for 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.
I'll add some unit tests this week 👍🏻
Signed-off-by: Kevin <kpostlet@redhat.com>
67e318f
to
509ced3
Compare
@d4l3k I've added those unit tests. Feel free to merge if you think the PR is good to go |
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.
LGTM thanks for adding!
…y configurable
I also added a note to each schedulers init method to help with maintainablility
I changed the create scheduler function for the local scheduler to specify the key word args that it takes rather than getting them from the
**kwargs
dict.Test plan: