Skip to content
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

[autoscaler] Fix worker capping fifo test in new scheduler #12512

Merged
merged 10 commits into from
Dec 4, 2020

Conversation

ericl
Copy link
Contributor

@ericl ericl commented Dec 1, 2020

In the new scheduler, workers are popped more aggressively. This isn't incorrect, but breaks the test. Fix the test by changing the condition to be a bit more relaxed.

Closes #12435

Copy link
Member

@kfstorm kfstorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. One comment.

python/ray/tests/test_multi_tenancy.py Outdated Show resolved Hide resolved
@ericl
Copy link
Contributor Author

ericl commented Dec 1, 2020

Test still failing

@ericl ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Dec 1, 2020
@kfstorm
Copy link
Member

kfstorm commented Dec 2, 2020

Still failing

@ericl
Copy link
Contributor Author

ericl commented Dec 3, 2020

Still seems failing due to travis not starting 3 workers when num_cpus=3 somehow. I'm inclined to remove this test since it's inherently brittle, and should be unit tested instead (though not sure if we currently have such a unit test). @kfstorm does this sound ok?

@kfstorm
Copy link
Member

kfstorm commented Dec 3, 2020

@ericl It sounds good to me. But can you add a test in worker_pool_test in this PR rather than just removing this test?

@ericl
Copy link
Contributor Author

ericl commented Dec 3, 2020

@kfstorm I don't have the bandwidth to do that, we can also ignore the test for now and go ahead with the new scheduler regardless

@kfstorm
Copy link
Member

kfstorm commented Dec 3, 2020

@ericl OK, I'll add the test into worker_pool_test.

@ericl ericl merged commit 8cebe1e into ray-project:master Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[New scheduler] Fix test_multi_tenancy::test_worker_capping_fifo
3 participants