[pull] master from ray-project:master - #1191
Merged
Merged
Conversation
…a CI timeout (#65109) ## Why are these changes needed? `//python/ray/tests:test_generators` and `//python/ray/dag:tests/experimental/test_compiled_graphs` are the two worst offenders in the RocksDB GCS premerge job. Both are reported as TIMEOUT/FLAKY at the *target* level, consuming the full Bazel budget on every attempt, even though the underlying failure is a single test case. Comparing premerge [#70949](https://buildkite.com/ray-project/premerge/builds/70949), which ran the redis and rocksdb jobs on the same commit, the suite totals are effectively identical (**29,427s** redis vs **29,644s** rocksdb, +0.7%), so there is no general backend latency tax. The damage is concentrated: | target | redis | rocksdb | ratio | | |---|---|---|---|---| | `//python/ray/dag:.../test_compiled_graphs` | 1193.6s | 3600.2s | 3.02× | FLAKY | | `//python/ray/tests:test_generators` | 416.7s | 915.1s | 2.20× | TIMEOUT | | `//python/ray/tests:test_multi_node_3` | 162.1s | 207.9s | 1.28× | passing | Everything else is ≤1.5×. And it is not a timing-margin problem: the retry of the same rocksdb shard passed `test_generators` in **343.7s**, i.e. *faster* than redis. The distribution is bimodal, which points at a wedge rather than a slowdown. Reading the timeout dumps, both targets wedge on an unbounded wait that has nothing to do with the assertion under test: 1. `Node._kill_process_type` waits with `timeout=None` whenever the caller passes `wait=True`, which `Cluster.remove_node` always does. `SIGKILL` cannot reap a process parked in uninterruptible sleep, and a process blocked in the `fsync` that the RocksDB GCS issues on every write is exactly that. So `ray_start_cluster` teardown blocks forever. In the failing `test_generators` attempt, pytest-timeout fired at 180s and teardown then absorbed the remaining ~700s until Bazel killed the target at 900s — twice, because of `--flaky_test_attempts=2`. One test-case failure cost **30 minutes of CI** and was reported as TIMEOUT instead of a clean FAILED-then-retry. 2. `run_string_as_driver` / `run_string_as_driver_stdout_stderr` call `proc.communicate()` with no timeout, so a driver that hangs during shutdown blocks the test forever. That is precisely what `test_compiled_graphs::test_async_shutdown` does, and it is the point where that target's timeout dump lands. Neither wait is load-bearing: nothing depends on waiting *forever*, only on waiting long enough. ## What this changes - **`Node._kill_process_type`**: bound the post-SIGKILL wait at 30s even when `wait=True`, and log the pid and process type when it expires. Reaping is normally instantaneous, so this is inert in the healthy case; when it does expire, `Cluster.remove_node`'s existing `any_processes_alive()` assertion now reports a real error in seconds instead of hanging. - **`run_string_as_driver` / `run_string_as_driver_stdout_stderr`**: add a `timeout` parameter defaulting to 300s. On expiry, kill the driver, log whatever it produced, and re-raise `TimeoutExpired`. 300s is well above the 180s pytest-timeout that already governs almost every caller, so no existing blocking driver should be affected. Pass `timeout=None` to restore the old behaviour. This makes the failures **bounded and attributable**. It deliberately does not attempt to fix the underlying test-case flake, which is still under investigation and has not been reproduced outside CI — 48/48 local runs of the four `test_dynamic_generator_reconstruction_nondeterministic` variants passed under both backends, with rocksdb showing no slowdown (median **68.9s** vs **70.2s** for the in-memory GCS). ## Related issue number Follow-up to #64702 (REP-64). Not a duplicate — I checked open PRs and none touch these two waits. ## Checks - [x] I've signed off every commit (DCO). - [x] I've made sure the tests are passing. Tested locally: - `test_dynamic_generator_reconstruction_nondeterministic[None-False]` and `[None-True]` under `TEST_GCS_ROCKSDB=1`: **2 passed in 136.9s** - `test_output.py -k test_disable_driver_logs_breakpoint`: **1 passed** - direct exercise of all three `run_string_as_driver*` paths, including the new timeout path (kills the driver and raises `TimeoutExpired`) - `pre-commit run` clean on all three changed files AI assistance (GitHub Copilot CLI) was used for the CI log analysis and to draft these changes; every line was reviewed by me. --------- Signed-off-by: Santosh Jha <santosh.m.jha@gmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Rueian <rueiancsie@gmail.com>
## Description Re-opening of #63640 since GitHub closed it due to a force-push base change. Regenerated the `package-lock.json` using Node 14.21.3 as requested by @sampan-s-nayak. Bumped axios to `^1.18.0` to fully address GHSA-gcfj-64vw-6mp9. ## Related issues Closes #63640 ## Additional information Signed-off-by: RinZ27 <222222878+RinZ27@users.noreply.github.com> --- *Note: This is a replacement for PR #65106 which was automatically closed due to a git history issue.* Signed-off-by: RinZ27 <222222878+RinZ27@users.noreply.github.com>
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )