[serve] Stabilize metrics pusher #38349
Merged
Conversation
This file contains 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
zcin
commented
Aug 11, 2023
python/ray/serve/tests/test_util.py
Outdated
| @@ -665,6 +667,21 @@ def test_get_all_live_placement_group_names(ray_instance): | |||
| assert set(get_all_live_placement_group_names()) == {"pg3", "pg4", "pg5", "pg6"} | |||
|
|
|||
|
|
|||
| def test_metrics_pusher(): | |||
There was a problem hiding this comment.
Without this change, this test almost never passes. counter["val"] at the end of the test is 16-17.
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
|
Nice to catch this issue! (make our autoscaler more robust!) sync offline to respect last success time to sleep. |
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
edoakes
requested changes
Aug 11, 2023
There was a problem hiding this comment.
Nice fix! This approach looks good to me, pending @edoakes's comment.
|
|
||
| # For all tasks, check when the task should be executed | ||
| # next. Sleep until the next closest time. | ||
| least_interval_s = math.inf |
There was a problem hiding this comment.
[Nit] Could we raise an error if the MetricsPusher is started without any tasks registered? As written, it silently sleeps forever since least_interval_s gets set to math.inf.
There was a problem hiding this comment.
That makes sense! I've added this to MetricsPusher.start()
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
edoakes
approved these changes
Aug 11, 2023
shrekris-anyscale
approved these changes
Aug 11, 2023
| for _ in range(10000000): | ||
| for key in result.keys(): | ||
| assert result[key] == expected_results[key] | ||
| if len(result) == 3: |
There was a problem hiding this comment.
Should the test fail if len(result) is never 3? We should add an assertion after the for loop in that case. Same question for test_metrics_pusher_basic.
There was a problem hiding this comment.
Yeah good call, I've added this in both tests!
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
edoakes
approved these changes
Aug 14, 2023
NripeshN
pushed a commit
to NripeshN/ray
that referenced
this pull request
Aug 15, 2023
This PR solves two problems. First, suppose there is one task registered on the metrics pusher with interval 2 seconds. What the metrics pusher does repeatedly: 1. Record `start = time.time()` 2. Execute task 3. Record task `last_call_succeeded_time` 9. Sleep until `time = start + 2` The problem: Strictly speaking, `start + 2 < task.last_call_succeeded_time` because `start < task.last_call_succeeded_time`. This is not always a problem because the task execution is very fast (1-5ms) and `time.sleep(2)` sleeps for "at least 2 seconds", so oftentimes the thread actually wakes up at a time that is after `task.last_call_succeeded_time + 2`. However sometimes the thread wakes up before `task.last_call_succeeded_time + 2` meaning it doesn't satisfy the if statement and doesn't execute the task to push the metric. This causes the metrics pusher to "skip" intervals. Since all our tasks and callback functions are essentially no-ops, we should just sleep until at least `last_call_succeeded_time + 2`. Second, suppose there is more than one task registered on the shared metrics pusher, one with interval 8s and one with interval 10s. Currently, the thread on the metrics pusher will wake up every 8 seconds to check if any tasks need executing. This means the first task will be executed every 8 seconds, and the second task every 16 seconds. Instead of setting `least_interval_s` to `min(task.interval_s for all tasks)`, we should check at every interval when is the next time any task needs to be executed, so the sleep time is always varying. E.g. with an 8s interval task and 10s interval task, the thread should roughly sleep 8 seconds then 2 seconds and repeat. Signed-off-by: NripeshN <nn2012@hw.ac.uk>
harborn
pushed a commit
to harborn/ray
that referenced
this pull request
Aug 17, 2023
This PR solves two problems. First, suppose there is one task registered on the metrics pusher with interval 2 seconds. What the metrics pusher does repeatedly: 1. Record `start = time.time()` 2. Execute task 3. Record task `last_call_succeeded_time` 9. Sleep until `time = start + 2` The problem: Strictly speaking, `start + 2 < task.last_call_succeeded_time` because `start < task.last_call_succeeded_time`. This is not always a problem because the task execution is very fast (1-5ms) and `time.sleep(2)` sleeps for "at least 2 seconds", so oftentimes the thread actually wakes up at a time that is after `task.last_call_succeeded_time + 2`. However sometimes the thread wakes up before `task.last_call_succeeded_time + 2` meaning it doesn't satisfy the if statement and doesn't execute the task to push the metric. This causes the metrics pusher to "skip" intervals. Since all our tasks and callback functions are essentially no-ops, we should just sleep until at least `last_call_succeeded_time + 2`. Second, suppose there is more than one task registered on the shared metrics pusher, one with interval 8s and one with interval 10s. Currently, the thread on the metrics pusher will wake up every 8 seconds to check if any tasks need executing. This means the first task will be executed every 8 seconds, and the second task every 16 seconds. Instead of setting `least_interval_s` to `min(task.interval_s for all tasks)`, we should check at every interval when is the next time any task needs to be executed, so the sleep time is always varying. E.g. with an 8s interval task and 10s interval task, the thread should roughly sleep 8 seconds then 2 seconds and repeat. Signed-off-by: harborn <gangsheng.wu@intel.com>
harborn
pushed a commit
to harborn/ray
that referenced
this pull request
Aug 17, 2023
This PR solves two problems. First, suppose there is one task registered on the metrics pusher with interval 2 seconds. What the metrics pusher does repeatedly: 1. Record `start = time.time()` 2. Execute task 3. Record task `last_call_succeeded_time` 9. Sleep until `time = start + 2` The problem: Strictly speaking, `start + 2 < task.last_call_succeeded_time` because `start < task.last_call_succeeded_time`. This is not always a problem because the task execution is very fast (1-5ms) and `time.sleep(2)` sleeps for "at least 2 seconds", so oftentimes the thread actually wakes up at a time that is after `task.last_call_succeeded_time + 2`. However sometimes the thread wakes up before `task.last_call_succeeded_time + 2` meaning it doesn't satisfy the if statement and doesn't execute the task to push the metric. This causes the metrics pusher to "skip" intervals. Since all our tasks and callback functions are essentially no-ops, we should just sleep until at least `last_call_succeeded_time + 2`. Second, suppose there is more than one task registered on the shared metrics pusher, one with interval 8s and one with interval 10s. Currently, the thread on the metrics pusher will wake up every 8 seconds to check if any tasks need executing. This means the first task will be executed every 8 seconds, and the second task every 16 seconds. Instead of setting `least_interval_s` to `min(task.interval_s for all tasks)`, we should check at every interval when is the next time any task needs to be executed, so the sleep time is always varying. E.g. with an 8s interval task and 10s interval task, the thread should roughly sleep 8 seconds then 2 seconds and repeat.
arvind-chandra
pushed a commit
to lmco/ray
that referenced
this pull request
Aug 31, 2023
This PR solves two problems. First, suppose there is one task registered on the metrics pusher with interval 2 seconds. What the metrics pusher does repeatedly: 1. Record `start = time.time()` 2. Execute task 3. Record task `last_call_succeeded_time` 9. Sleep until `time = start + 2` The problem: Strictly speaking, `start + 2 < task.last_call_succeeded_time` because `start < task.last_call_succeeded_time`. This is not always a problem because the task execution is very fast (1-5ms) and `time.sleep(2)` sleeps for "at least 2 seconds", so oftentimes the thread actually wakes up at a time that is after `task.last_call_succeeded_time + 2`. However sometimes the thread wakes up before `task.last_call_succeeded_time + 2` meaning it doesn't satisfy the if statement and doesn't execute the task to push the metric. This causes the metrics pusher to "skip" intervals. Since all our tasks and callback functions are essentially no-ops, we should just sleep until at least `last_call_succeeded_time + 2`. Second, suppose there is more than one task registered on the shared metrics pusher, one with interval 8s and one with interval 10s. Currently, the thread on the metrics pusher will wake up every 8 seconds to check if any tasks need executing. This means the first task will be executed every 8 seconds, and the second task every 16 seconds. Instead of setting `least_interval_s` to `min(task.interval_s for all tasks)`, we should check at every interval when is the next time any task needs to be executed, so the sleep time is always varying. E.g. with an 8s interval task and 10s interval task, the thread should roughly sleep 8 seconds then 2 seconds and repeat. Signed-off-by: e428265 <arvind.chandramouli@lmco.com>
vymao
pushed a commit
to vymao/ray
that referenced
this pull request
Oct 11, 2023
This PR solves two problems. First, suppose there is one task registered on the metrics pusher with interval 2 seconds. What the metrics pusher does repeatedly: 1. Record `start = time.time()` 2. Execute task 3. Record task `last_call_succeeded_time` 9. Sleep until `time = start + 2` The problem: Strictly speaking, `start + 2 < task.last_call_succeeded_time` because `start < task.last_call_succeeded_time`. This is not always a problem because the task execution is very fast (1-5ms) and `time.sleep(2)` sleeps for "at least 2 seconds", so oftentimes the thread actually wakes up at a time that is after `task.last_call_succeeded_time + 2`. However sometimes the thread wakes up before `task.last_call_succeeded_time + 2` meaning it doesn't satisfy the if statement and doesn't execute the task to push the metric. This causes the metrics pusher to "skip" intervals. Since all our tasks and callback functions are essentially no-ops, we should just sleep until at least `last_call_succeeded_time + 2`. Second, suppose there is more than one task registered on the shared metrics pusher, one with interval 8s and one with interval 10s. Currently, the thread on the metrics pusher will wake up every 8 seconds to check if any tasks need executing. This means the first task will be executed every 8 seconds, and the second task every 16 seconds. Instead of setting `least_interval_s` to `min(task.interval_s for all tasks)`, we should check at every interval when is the next time any task needs to be executed, so the sleep time is always varying. E.g. with an 8s interval task and 10s interval task, the thread should roughly sleep 8 seconds then 2 seconds and repeat. Signed-off-by: Victor <vctr.y.m@example.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Why are these changes needed?
This PR solves two problems.
First, suppose there is one task registered on the metrics pusher with interval 2 seconds.
What the metrics pusher does repeatedly:
start = time.time()last_call_succeeded_timetime = start + 2The problem:
Strictly speaking,
start + 2 < task.last_call_succeeded_timebecausestart < task.last_call_succeeded_time. This is not always a problem because the task execution is very fast (1-5ms) andtime.sleep(2)sleeps for "at least 2 seconds", so oftentimes the thread actually wakes up at a time that is aftertask.last_call_succeeded_time + 2. However sometimes the thread wakes up beforetask.last_call_succeeded_time + 2meaning it doesn't satisfy the if statement and doesn't execute the task to push the metric. This causes the metrics pusher to "skip" intervals.Since all our tasks and callback functions are essentially no-ops, we should just sleep until at least
last_call_succeeded_time + 2.Second, suppose there is more than one task registered on the shared metrics pusher, one with interval 8s and one with interval 10s. Currently, the thread on the metrics pusher will wake up every 8 seconds to check if any tasks need executing. This means the first task will be executed every 8 seconds, and the second task every 16 seconds.
Instead of setting
least_interval_stomin(task.interval_s for all tasks), we should check at every interval when is the next time any task needs to be executed, so the sleep time is always varying. E.g. with an 8s interval task and 10s interval task, the thread should roughly sleep 8 seconds then 2 seconds and repeat.Related issue number
Closes #38360
Closes #38361
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.