[data] Skip upscaling validation warning for fixed-size actor pools#60569
[data] Skip upscaling validation warning for fixed-size actor pools#60569bveeramani merged 1 commit intomasterfrom
Conversation
Signed-off-by: Jeffrey Wang <[email protected]>
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request effectively addresses the issue of unnecessary autoscaling validation warnings for fixed-size actor pools by introducing an early exit condition in the _validate_actor_pool_autoscaling_config function. A new test case has been added to test_autoscaler.py to verify this behavior, which is a good practice. The core logic change is sound and the test covers the intended functionality.
| expected_message = ( | ||
| "⚠️ Actor Pool configuration of the " | ||
| "ActorPoolMapOperator[MapBatches(SimpleMapper)] will not allow it to scale up: " |
There was a problem hiding this comment.
The warning message prefix "⚠️ Actor Pool configuration of the ActorPoolMapOperator[MapBatches(SimpleMapper)] will not allow it to scale up: " is duplicated across multiple test cases within this function (e.g., Test #1, Test #2, Test #3, and this new Test #4). Defining this common prefix as a constant at the function level would improve maintainability and reduce repetition, making it easier to update if the warning message format changes in the future.
| expected_message = ( | |
| "⚠️ Actor Pool configuration of the " | |
| "ActorPoolMapOperator[MapBatches(SimpleMapper)] will not allow it to scale up: " | |
| _COMMON_WARNING_PREFIX = ( | |
| "⚠️ Actor Pool configuration of the " | |
| "ActorPoolMapOperator[MapBatches(SimpleMapper)] will not allow it to scale up: " | |
| ) | |
| expected_message = _COMMON_WARNING_PREFIX |
bveeramani
left a comment
bveeramani
left a comment
There was a problem hiding this comment.
Out-of-scope for this PR, but I think we should refactor test_autoscaling_config_validation_warnings to be a unit test, and also split it up into multiple cases.
When you write your tests, try to include only one Act task per test. Some common approaches for implementing a single Act task include creating a separate test for each Act or using parameterized tests. There are several benefits to using a single Act task for each test:
You can easily discern which Act task is failing if the test fails.
You can ensure the test is focused on just a single case.
You gain a clear picture as to why your tests are failing.
Multiple Act tasks need to be individually asserted, and you can't guarantee that all Assert tasks execute. In most unit testing frameworks, after an Assert task fails in a unit test, all subsequent tests are automatically considered as failing. The process can be confusing because some working functionality might be interpreted as failing.
…ay-project#60569) ## Description The autoscaling validation warning was incorrectly raised for fixed-size actor pools (`min_size == max_size`). These pools don't scale up, so the warning doesn't apply. ## Related issues Context: ray-project#60477 (comment) ## Additional information After this change, when we run `python -m pytest -v -s test_vllm_engine_proc.py::test_generation_model`, we no longer observe autoscaling warnings in the log. Signed-off-by: Jeffrey Wang <[email protected]> Signed-off-by: jinbum-kim <[email protected]>
…ay-project#60569) ## Description The autoscaling validation warning was incorrectly raised for fixed-size actor pools (`min_size == max_size`). These pools don't scale up, so the warning doesn't apply. ## Related issues Context: ray-project#60477 (comment) ## Additional information After this change, when we run `python -m pytest -v -s test_vllm_engine_proc.py::test_generation_model`, we no longer observe autoscaling warnings in the log. Signed-off-by: Jeffrey Wang <[email protected]>
…ay-project#60569) ## Description The autoscaling validation warning was incorrectly raised for fixed-size actor pools (`min_size == max_size`). These pools don't scale up, so the warning doesn't apply. ## Related issues Context: ray-project#60477 (comment) ## Additional information After this change, when we run `python -m pytest -v -s test_vllm_engine_proc.py::test_generation_model`, we no longer observe autoscaling warnings in the log. Signed-off-by: Jeffrey Wang <[email protected]> Signed-off-by: 400Ping <[email protected]>
…ay-project#60569) ## Description The autoscaling validation warning was incorrectly raised for fixed-size actor pools (`min_size == max_size`). These pools don't scale up, so the warning doesn't apply. ## Related issues Context: ray-project#60477 (comment) ## Additional information After this change, when we run `python -m pytest -v -s test_vllm_engine_proc.py::test_generation_model`, we no longer observe autoscaling warnings in the log. Signed-off-by: Jeffrey Wang <[email protected]> Signed-off-by: Adel Nour <[email protected]>
…ay-project#60569) ## Description The autoscaling validation warning was incorrectly raised for fixed-size actor pools (`min_size == max_size`). These pools don't scale up, so the warning doesn't apply. ## Related issues Context: ray-project#60477 (comment) ## Additional information After this change, when we run `python -m pytest -v -s test_vllm_engine_proc.py::test_generation_model`, we no longer observe autoscaling warnings in the log. Signed-off-by: Jeffrey Wang <[email protected]> Signed-off-by: peterxcli <[email protected]>
…ay-project#60569) ## Description The autoscaling validation warning was incorrectly raised for fixed-size actor pools (`min_size == max_size`). These pools don't scale up, so the warning doesn't apply. ## Related issues Context: ray-project#60477 (comment) ## Additional information After this change, when we run `python -m pytest -v -s test_vllm_engine_proc.py::test_generation_model`, we no longer observe autoscaling warnings in the log. Signed-off-by: Jeffrey Wang <[email protected]> Signed-off-by: peterxcli <[email protected]>
2 participants
Description
The autoscaling validation warning was incorrectly raised for fixed-size actor pools (
min_size == max_size). These pools don't scale up, so the warning doesn't apply.Related issues
Context: #60477 (comment)
Additional information
After this change, when we run
python -m pytest -v -s test_vllm_engine_proc.py::test_generation_model, we no longer observe autoscaling warnings in the log.