Skip to content

[Data] batch-meta-fetch + core end-of-stream timeout + ExecutionResources/budget-loops perf (combined, for testing) - #64018

Closed
xinyuangui2 wants to merge 31 commits into
ray-project:masterfrom
xinyuangui2:combo-core-63701-63964
Closed

[Data] batch-meta-fetch + core end-of-stream timeout + ExecutionResources/budget-loops perf (combined, for testing)#64018
xinyuangui2 wants to merge 31 commits into
ray-project:masterfrom
xinyuangui2:combo-core-63701-63964

Conversation

@xinyuangui2

Copy link
Copy Markdown
Contributor

Combined branch for testing/benchmarking only — not for review.

= #63701 (background-thread metadata fetch) + #64014 (core end-of-stream timeout fix) + #63964 (ExecutionResources __slots__/combine_sum + raw-float reservation/budget loops).

🤖 Generated with Claude Code

xinyuangui2 and others added 30 commits May 28, 2026 17:39
… iter

Per-pair ray.get(meta_ref) in DataOpTask.on_data_ready is the largest
leaf cost in process_completed_tasks at high scale (~1-2 ms fixed
overhead per call, hits the raylet RPC + msgpack wrapper). At 5000
workers with ~10 blocks each per scheduler tick, that's ~50 k pairs
× ~1.5 ms ≈ 75 s of leaf time amortizable into one batched call per
iteration.

Approach (no Core API change required, no streaming-gen protocol
change):

1. DataOpTask.peek_pending_meta_ref() pulls the next
   (block_ref, meta_ref) pair into the pending slots without
   fetching metadata. Returns the meta_ref so the scheduler-loop
   driver can batch it; subsequent on_data_ready consumes the same
   pending slots.

2. process_completed_tasks peeks each ready DataOpTask, asks the
   local CoreWorker (via ray.experimental.get_local_object_locations
   — local-only, no RPC) which meta_refs have a non-None
   ``object_size``. A non-None size is the load-bearing signal that
   the owner has processed the task return for that ref and a
   ray.get won't block.

3. Refs with known size go into ONE batched ray.get; the resulting
   dict is passed to each task's on_data_ready via a new
   ``prefetched_meta`` kwarg.

4. on_data_ready consults the dict first and falls back to the
   existing per-ref ray.get + GetTimeoutError warning when the dict
   misses — preserving all error semantics for refs whose size
   wasn't yet known (rare warmup window).

Also adds a debug-log emitted every 100 iterations summarizing the
size-known rate (e.g. "82 / 90 (91.1%) had known size and were
batched"). The rate tells us how much of the win we're actually
capturing vs falling through to the per-ref path.

Tests: three new mock-friendly tests in TestDataOpTask exercise
peek_pending_meta_ref, the prefetched-meta hit path, and the empty-
dict fallback to the per-ref path.

No behavior change for refs that miss the local-locations check; the
existing per-ref path with its timeout + worker-crash warning still
catches them.

Signed-off-by: xgui <xgui@anyscale.com>
The previous revision of this PR queried get_local_object_locations
on the meta_ref. That returns the size of the pickled metadata blob
(a few KB) — not a useful signal for the budget loop in
on_data_ready, which compares against meta.size_bytes (the block's
data size).

Switch to querying the block_ref's object_size:

- block_ref.object_size is the block's actual data size in plasma,
  which is what maps to meta.size_bytes in the budget loop.
- A known block size also implies the worker has finished producing
  the block AND yielded the meta_ref (streaming gens yield block
  then meta), so the batched ray.get(meta_refs) is safe.

Rename peek_pending_meta_ref -> peek_pending_pair returning the full
(block_ref, meta_ref) tuple so the caller can inspect block size and
still address the meta_ref for the batched fetch.

Updated tests accordingly.

Signed-off-by: xgui <xgui@anyscale.com>
…pass

Restructures process_completed_tasks into a 3-phase pipeline that
preserves bit-identical emission order while collapsing N per-pair
ray.get(meta_ref) calls into one batched call per scheduler iter.

1. Per-op loop (unchanged order): each DataOpTask.on_data_ready
   appends every pulled (block_ref, meta_ref) pair to a shared
   deferred: List[DeferredEmit]. No RefBundle is emitted inside
   on_data_ready in this mode; the loop only manages budget.

   - Known block size (from ray.experimental.get_local_object_locations,
     local-only no-RPC lookup): defer with meta_bytes=None; budget
     uses the local object_size.
   - Unknown block size: synchronous per-ref ray.get(meta_ref, timeout)
     to obtain size for budget; stash the bytes on the deferred entry
     so the caller doesn't refetch. Per-ref GetTimeoutError warning
     and retry-next-iter semantics preserved exactly.

2. _replay_deferred_emits: one batched ray.get covering deferred
   entries with meta_bytes=None. On failure: per-ref retry fallback,
   then skip refs that still fail with a warning. Replay deferred
   list in append order, calling _output_ready_callback and updating
   _last_block_meta — emission order matches today's per-op,
   per-task, per-pair traversal exactly.

3. Postponed task_done_callback: when the streaming gen raises
   StopIteration during on_data_ready in deferred mode, we set
   _task_done_pending instead of firing immediately, so
   _last_block_meta has time to reach its final value during replay.
   After replay, _replay_deferred_emits fires task_done_callback for
   any task with the flag set.

Removes peek_pending_pair (API) and _batched_fetch_meta (helper) —
the new design does the local lookup per-pair inside on_data_ready
instead of cross-op peeking.

Debug log every ~100 scheduler iterations summarizes the size-known
rate. Flip the data logger to DEBUG to see it.

Legacy mode (deferred_emits=None) preserves the original behavior
unchanged for direct callers in tests and utilities.

Three tests in TestDataOpTask cover the legacy path, the deferred
path with replay, and emission-order preservation across multiple
tasks.

Signed-off-by: xgui <xgui@anyscale.com>
deferred_emits is now a required parameter. Test callers route through
the new DataOpTask.drain_and_emit helper (= deferred + sync replay in
one call) so on_data_ready has a single code path.

Signed-off-by: xgui <xgui@anyscale.com>
Signed-off-by: xgui <xgui@anyscale.com>

# Conflicts:
#	python/ray/data/_internal/execution/interfaces/physical_operator.py
…obe stats

Update 1 on the deferred-emit pipeline:

- On a local-size miss, estimate the output budget from the operator's
  running average block size (recorded from exact meta.size_bytes in
  replay) instead of a per-ref ray.get. All pairs now defer
  (meta_bytes=None) so metadata flows through ONE batched ray.get — a
  single metadata-fetch path. Misses are rare (object_size is locally
  known for ~all pairs), so the estimate is a small, transient budget
  approximation; the exact metadata still arrives via the batched fetch.

- Replace the old size-known counter (which keyed off meta_bytes, now
  always None) with the ray-project#63904-style probe recorded in replay_deferred_emits
  where both the local object_size and exact meta.size_bytes are known:
  hit rate, object_size/meta ratio, within-1%, and p50/p90/max relative
  diff. Logged periodically and surfaced in the worker_scaling result
  metrics (local_size_probe_*).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: xgui <xgui@anyscale.com>
Move the batched `ray.get(meta_refs)` for deferred RefBundle emits off the
scheduling-loop thread into a dedicated `MetadataPrefetcher` thread, so the
executor thread never blocks on metadata fetches.

- New `MetadataPrefetcher`: executor thread calls `submit()` (enqueue an op's
  deferred meta_refs + append to a per-op FIFO) then `drain()` (emit pairs
  whose metadata is back, front-first per op). A background thread blocks on
  `ray.get`, coalescing queued batches into one fetch.
- Per-op in-order emit: each operator's FIFO is emitted front-first and stops
  at the first pair still in flight, so RefBundle emission order is preserved
  exactly; an op waiting on metadata is skipped this round and retried next
  (matches the synchronous break-and-retry). Ops are independent.
- Postponed `task_done_callback` fires only once a task's pending emits reach
  zero (`_pending_emit_count`), so a task stays active until fully drained.
- `process_completed_tasks` takes an optional `metadata_prefetcher`; without
  one it falls back to the synchronous batched get + `replay_deferred_emits`.
  StreamingExecutor creates/starts/stops the prefetcher.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: xgui <xgui@anyscale.com>
The deferred-emit path constructed RefBundle with a legacy (ref, metadata)
tuple, but RefBundle.blocks now requires BlockEntry instances (upstream
replaced the 2-tuple shape). Wrap the block in BlockEntry so the emit
helper used by both replay_deferred_emits and the MetadataPrefetcher
produces a valid bundle.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: xgui <xgui@anyscale.com>
The streaming scheduler constructs ExecutionResources on its hot path
millions of times per run (every add/subtract/max/copy returns a new
object). Profiling a 15-operator workload showed ~44% of the scheduling
loop in execution_options.py, with ExecutionResources.__setattr__ alone
at ~18%.

1. Give ExecutionResources __slots__ and drop the custom __setattr__
   immutability guard. Attribute access goes through slot descriptors
   (no per-instance __dict__) and construction no longer pays a Python
   __setattr__ call + dict-membership check per field. Immutability is
   upheld by convention (fields set once in __init__; only the lazy
   _quantized cache transitions None -> tuple).

2. Add ExecutionResources.combine_sum() and use it for the per-iteration
   usage/budget rollups (global usage totals in update_usages, the
   completed-ops and downstream-ineligible folds). It sums raw floats in
   one pass and allocates a single result instead of one intermediate
   ExecutionResources per operator, removing O(num_ops) allocations from
   the update_usages -> _update_allocated_budgets path.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: xgui <xgui@anyscale.com>
Empty folds are common on the hot path (completed-ops and
downstream-ineligible usage rollups are empty on most iterations);
reuse the shared zero() instead of allocating a fresh instance.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: xgui <xgui@anyscale.com>
Signed-off-by: xgui <xgui@anyscale.com>

# Conflicts:
#	python/ray/data/_internal/execution/interfaces/physical_operator.py
#	release/nightly_tests/dataset/worker_scaling_benchmark.py
…lback and probe

- on_data_ready: the driver owns every block ref the streaming generator
  yields, so get_local_object_locations always knows object_size — assert
  it instead of estimating, and drop the per-op running-average fallback.
- Remove the local-size probe telemetry (hit rate / size-diff counters)
  and its worker_scaling_benchmark hooks.
- Deferred emit via MetadataPrefetcher is now the only mode: remove the
  synchronous replay fallback (replay_deferred_emits / drain_and_emit);
  make metadata_prefetcher required in process_completed_tasks. Tests use
  a new MetadataPrefetcher.flush() + a sync drive helper in tests/util.py.
- Remove stray python/ray/thirdparty_files artifact.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: xgui <xgui@anyscale.com>
ReservationOpResourceAllocator._update_reservation and update_budgets run
on every scheduling iteration and chained ExecutionResources arithmetic
(subtract/max/min/add/scale), allocating ~20 intermediate objects per
operator per iteration. Rewrite the loops to accumulate raw floats per
resource dimension and materialize ExecutionResources only for the stored
results (_op_reserved, _total_shared, _op_budgets).

Quantized comparisons (satisfies_limit / is_zero / is_non_negative
equivalents) use safe_round with the same digits as
ExecutionResources._quantized_key, so behavior is unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: xgui <xgui@anyscale.com>
…on overhead

bytes_read is now the block's object-store object_size, which is a few KB
larger than meta.size_bytes (per-object serialization overhead), so the
exact pytest.approx default tolerance is too tight.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: xgui <xgui@anyscale.com>
Drain each task in a loop until end-of-stream: on a 1-CPU cluster the
second stub generator hasn't started when the first is ready, so a single
on_data_ready call pulls nothing from it. The failure also leaked a
backpressured generator (held by the pytest traceback) that pinned the
only CPU and hung every later test needing a task.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: xgui <xgui@anyscale.com>
…tations; hoist local imports

- MetadataPrefetcher fetch thread: accumulate meta_refs in a pending set,
  ray.wait(num_returns=all, timeout=0.1, fetch_local=True), and ray.get only
  the ready refs — a ref stuck on a bad node stays pending instead of
  blocking the whole thread and starving other operators' metadata.
- Deferred-emit tests: block sizes now carry a small format overhead
  (+8 bytes after the master merge); compare with pytest.approx(abs=64).
- Hoist function-local imports (BlockMetadataWithSchema, DeferredEmit,
  _emit_deferred_entry, MetadataPrefetcher) to module level; no cycles.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: xgui <xgui@anyscale.com>
Restore the pre-deferred gating semantics, without blocking calls:

- A task whose previously pulled pairs are still awaiting their background
  metadata fetch (_pending_emit_count > 0) pulls nothing this iteration —
  the producer must not get arbitrarily far ahead of unfetched metadata.
- If a block's local object_size isn't known (e.g. lost to node failure),
  leave the pair pending and retry later instead of asserting/consuming.
  Consuming it advanced the generator stream to end-of-stream, whose
  handling blocks on the generator ref — with no node to reconstruct on,
  that deadlocked the scheduling thread (caught by
  test_on_data_ready_with_preemption_after_wait). A zero-timeout
  ray.wait(fetch_local=True) nudges reconstruction in the background.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: xgui <xgui@anyscale.com>
Waiting fetch_local on the block would pull full block data to the
driver, which is never needed. The metadata object is tiny, gets fetched
anyway once the pair is consumed, and reconstructing it re-executes the
same producer task, restoring the block too.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: xgui <xgui@anyscale.com>
The deadlock chain in test_on_data_ready_with_preemption_after_wait:
consuming the last pair while its objects were lost advanced the stream
to end-of-stream, where _next_sync ray.get()s the generator's return
object with no timeout — blocking the scheduling thread until a node
exists to reconstruct it. (Master has the same exposure in a much
narrower window: node death between the last metadata fetch and the next
probe.)

- on_data_ready: when the stream is exhausted
  (is_object_ref_stream_finished), only enter _next_sync's end-of-stream
  handling once the generator's return object is locally available
  (zero-timeout ray.wait probe); otherwise retry next iteration.
- tests/util.drain_and_emit: gate the metadata fetch behind ray.wait like
  the prefetcher; unfetched pairs stay in a caller-owned deferred list.
- preemption tests: consume-then-fetch charges the budget at consume time
  (the owner remembers a lost object's size), so assert one-time charging
  + no emission until reconstruction instead of bytes_read == 0.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: xgui <xgui@anyscale.com>
_next_sync documents 'if an object is not available within the given
timeout, it returns a nil object reference', but its end-of-stream
handling calls ray.get(generator_ref) with no timeout. If the return
object is remote — or lost to a failed node and pending reconstruction —
the get blocks the caller indefinitely. For Ray Data's scheduling thread
on a saturated cluster this deadlocks: reconstruction needs a CPU, the
CPUs are held by generator tasks blocked in output backpressure, and the
backpressure is only released by the blocked thread.

Apply the caller's timeout to the get and report a timeout as nil
(retry), per the documented contract. timeout_s=None (and -1) keep the
blocking behavior, so __next__ and other timeout-less callers are
unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: xgui <xgui@anyscale.com>
Reverts the three attempted deadlock fixes — the core _next_sync
end-of-stream timeout (b7188f8), the non-blocking end-of-stream guard
+ wait-gated test helper + preemption-test contract changes
(08fb7f4), and the lost-pair recovery nudge (1969902) — plus the
unknown-size break-and-retry branch, restoring the object_size asserts.

This intentionally returns the branch to the state where
test_on_data_ready_with_preemption_after_wait deadlocks, so the failure
mode can be described and a fix designed cleanly, rather than layering
point patches.

Kept (orthogonal to the deadlock): the prefetcher's ray.wait-gated fetch
loop, the _pending_emit_count run-ahead gate, import hoists, test
deflakes and size tolerances.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: xgui <xgui@anyscale.com>
Un-reverts the minimal fix set from d3630e1 (analysis posted on the
PR), leaving the end-of-stream guard out:

- [Core] _next_sync honors timeout_s in its end-of-stream ray.get and
  returns nil on timeout, per its documented contract. This is the
  categorical deadlock fix: a check-then-act race upstream now costs a
  bounded timeout + retry instead of an indefinite block. Private API;
  timeout_s=None/-1 callers (including __next__) are unchanged; Serve's
  to_object_ref now honors the user's timeout instead of blocking past
  it when a replica node dies.
- tests/util.drain_and_emit: metadata fetch gated behind ray.wait;
  unfetched pairs stay in a caller-owned deferred list.
- preemption tests: consume-then-fetch charges the budget once at
  consume time; assert no emission / no completion until reconstruction,
  and nothing blocks.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: xgui <xgui@anyscale.com>
…c too

Keeps the carried copy of the core fix identical to the standalone PR.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: xgui <xgui@anyscale.com>
- Drop the carried copy of the ObjectRefGenerator end-of-stream timeout
  fix; it lands separately in ray-project#64014 and will arrive here via a master
  merge. Skip test_on_data_ready_with_preemption_after_wait until then
  (it deterministically blocks without that fix).
- Add a test pinning the ordering guarantee for multiple outputs of one
  task: a later pair whose metadata is fetched first is held by the
  prefetcher's per-op FIFO until the earlier pair's metadata arrives, so
  outputs always emit in yield order.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: xgui <xgui@anyscale.com>
Expected to fail (block) until the ObjectRefGenerator end-of-stream
timeout fix (ray-project#64014) merges and arrives here via a master merge.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: xgui <xgui@anyscale.com>
_next_sync documents 'if an object is not available within the given
timeout, it returns a nil object reference', but its end-of-stream
handling calls ray.get(generator_ref) with no timeout to distinguish a
normal end of the stream from a task failure. If the return object is
remote — or lost to a failed node and pending lineage reconstruction —
the get blocks the caller indefinitely, ignoring the requested timeout.

A blocked caller can deadlock: reconstruction needs a CPU, and the
blocked caller may be exactly what releases one. Ray Data's scheduling
thread hits this on saturated clusters (the thread that frees
output-backpressured CPUs blocks waiting for a CPU); Serve's
to_object_ref can block past the user's requested timeout when a
replica node dies.

Apply the caller's timeout to the get and report a timeout as a nil ref
(retry), per the documented contract. timeout_s=None (and -1) keep the
blocking behavior, so __next__ and other timeout-less callers are
unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: xgui <xgui@anyscale.com>
_next_async has the same flaw as _next_sync: its next-item wait honors
timeout_s but the end-of-stream 'await self._generator_ref' does not.
Mirror the fix with asyncio.wait_for and extend the regression test to
the async path.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: xgui <xgui@anyscale.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a background metadata prefetcher (MetadataPrefetcher) to asynchronously fetch block metadata, preventing the main executor thread from blocking on ray.get. It also bounds end-of-stream gets in ObjectRefGenerator with timeouts to avoid deadlocks, and optimizes the scheduling hot path by replacing intermediate ExecutionResources allocations with raw float arithmetic. Feedback on these changes highlights several critical issues: potential nan propagation in resource_manager.py when performing arithmetic with float("inf") limits, a missing asyncio import in object_ref_generator.py that would trigger a NameError, and the lack of error handling or liveness checks for the new background prefetcher thread, which could cause the pipeline to hang if the thread dies silently.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +782 to +788
# remaining = max(remaining - (reserved_for_tasks + reserved_for_outputs), 0)
remaining_cpu = max(remaining_cpu - task_cpu, 0.0)
remaining_gpu = max(remaining_gpu - task_gpu, 0.0)
remaining_osm = max(
remaining_osm - (task_osm + reserved_for_outputs_osm), 0.0
)
remaining_mem = max(remaining_mem - task_mem, 0.0)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

If limits contains float("inf") (which is the default for unspecified limits), subtracting task_cpu (which will also be inf) from remaining_cpu (which is inf) results in nan. This nan value will propagate and break scheduling logic. We should guard the subtraction to only occur when the remaining resource is not inf.

Suggested change
# remaining = max(remaining - (reserved_for_tasks + reserved_for_outputs), 0)
remaining_cpu = max(remaining_cpu - task_cpu, 0.0)
remaining_gpu = max(remaining_gpu - task_gpu, 0.0)
remaining_osm = max(
remaining_osm - (task_osm + reserved_for_outputs_osm), 0.0
)
remaining_mem = max(remaining_mem - task_mem, 0.0)
# remaining = max(remaining - (reserved_for_tasks + reserved_for_outputs), 0)
if remaining_cpu != float("inf"):
remaining_cpu = max(remaining_cpu - task_cpu, 0.0)
if remaining_gpu != float("inf"):
remaining_gpu = max(remaining_gpu - task_gpu, 0.0)
if remaining_osm != float("inf"):
remaining_osm = max(
remaining_osm - (task_osm + reserved_for_outputs_osm), 0.0
)
if remaining_mem != float("inf"):
remaining_mem = max(remaining_mem - task_mem, 0.0)

Comment on lines +990 to 1003
# op_shared = min(op_shared, max(max_resource_usage - alloc, 0))
op_shared_cpu = min(
op_shared_cpu, max(max_resource_usage.cpu - alloc_cpu, 0.0)
)
op_shared_gpu = min(
op_shared_gpu, max(max_resource_usage.gpu - alloc_gpu, 0.0)
)
op_shared_osm = min(
op_shared_osm,
max(max_resource_usage.object_store_memory - alloc_osm, 0.0),
)
op_shared_mem = min(
op_shared_mem, max(max_resource_usage.memory - alloc_mem, 0.0)
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

When capping op_shared_cpu using max_resource_usage.cpu - alloc_cpu, if both are inf, it results in nan. This nan will propagate to op_shared_cpu and then to shared_cpu, breaking the budget allocation. We should only cap the shared allocation if the limit is not inf.

Suggested change
# op_shared = min(op_shared, max(max_resource_usage - alloc, 0))
op_shared_cpu = min(
op_shared_cpu, max(max_resource_usage.cpu - alloc_cpu, 0.0)
)
op_shared_gpu = min(
op_shared_gpu, max(max_resource_usage.gpu - alloc_gpu, 0.0)
)
op_shared_osm = min(
op_shared_osm,
max(max_resource_usage.object_store_memory - alloc_osm, 0.0),
)
op_shared_mem = min(
op_shared_mem, max(max_resource_usage.memory - alloc_mem, 0.0)
)
# op_shared = min(op_shared, max(max_resource_usage - alloc, 0))
if max_resource_usage.cpu != float("inf"):
op_shared_cpu = min(
op_shared_cpu, max(max_resource_usage.cpu - alloc_cpu, 0.0)
)
if max_resource_usage.gpu != float("inf"):
op_shared_gpu = min(
op_shared_gpu, max(max_resource_usage.gpu - alloc_gpu, 0.0)
)
if max_resource_usage.object_store_memory != float("inf"):
op_shared_osm = min(
op_shared_osm,
max(max_resource_usage.object_store_memory - alloc_osm, 0.0),
)
if max_resource_usage.memory != float("inf"):
op_shared_mem = min(
op_shared_mem, max(max_resource_usage.memory - alloc_mem, 0.0)
)

Comment on lines +1005 to +1008
shared_cpu -= op_shared_cpu
shared_gpu -= op_shared_gpu
shared_osm -= op_shared_osm
shared_mem -= op_shared_mem

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

When subtracting op_shared_cpu from shared_cpu, if shared_cpu is inf, subtracting op_shared_cpu (which will also be inf) results in nan. This nan will propagate to all subsequent operators' budgets. We should guard the subtraction to only occur when shared_cpu is not inf.

            if shared_cpu != float("inf"):
                shared_cpu -= op_shared_cpu
            if shared_gpu != float("inf"):
                shared_gpu -= op_shared_gpu
            if shared_osm != float("inf"):
                shared_osm -= op_shared_osm
            if shared_mem != float("inf"):
                shared_mem -= op_shared_mem

Comment on lines 5 to 9
from typing import TYPE_CHECKING, Deque, Iterator, Optional

import ray
from ray.exceptions import ObjectRefStreamEndOfStreamError
from ray.exceptions import GetTimeoutError, ObjectRefStreamEndOfStreamError
from ray.util.annotations import DeveloperAPI, PublicAPI

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

If asyncio is not imported in this file, calling _next_async will raise a NameError when attempting to use asyncio.wait_for or catch asyncio.TimeoutError.

Suggested change
from typing import TYPE_CHECKING, Deque, Iterator, Optional
import ray
from ray.exceptions import ObjectRefStreamEndOfStreamError
from ray.exceptions import GetTimeoutError, ObjectRefStreamEndOfStreamError
from ray.util.annotations import DeveloperAPI, PublicAPI
import asyncio
from typing import TYPE_CHECKING, Deque, Iterator, Optional
import ray
from ray.exceptions import GetTimeoutError, ObjectRefStreamEndOfStreamError
from ray.util.annotations import DeveloperAPI, PublicAPI

Comment on lines +176 to +220
def _run(self) -> None:
"""Fetch-thread loop: accumulate requested meta_refs into a pending
set, ``ray.wait`` (with a short timeout) for the ones that are
locally available, and ``ray.get`` + publish only those.

``ray.get`` is never issued on a ref that hasn't been reported ready
by ``ray.wait(fetch_local=True)``: a ref stuck on a bad/dead node
would otherwise block the whole thread forever and starve every
other operator's metadata. A stuck ref just stays in ``pending``
until Ray resolves or fails it.
"""
pending: List["ray.ObjectRef"] = []
while True:
# Block on the request queue only when there's nothing in
# flight; otherwise poll it so pending refs keep making progress.
if pending:
try:
item = self._request_q.get_nowait()
except queue_module.Empty:
item = ()
else:
item = self._request_q.get()
if item is None:
return
pending.extend(item)
# Coalesce any other already-queued batches.
while True:
try:
nxt = self._request_q.get_nowait()
except queue_module.Empty:
break
if nxt is None:
return
pending.extend(nxt)

if not pending:
continue
ready, pending = ray.wait(
pending,
num_returns=len(pending),
timeout=_FETCH_WAIT_TIMEOUT_S,
fetch_local=True,
)
if ready:
self._fetch(ready)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The background thread loop _run does not have any try-except block around its main loop. If an unexpected exception occurs (e.g., during ray.wait or queue operations), the thread will die silently. This will cause the entire Ray Data pipeline to hang indefinitely because drain() will always wait for results that will never be populated. Wrapping the loop in a try-except block to log the exception is highly recommended.

    def _run(self) -> None:
        """Fetch-thread loop: accumulate requested meta_refs into a pending
        set, ``ray.wait`` (with a short timeout) for the ones that are
        locally available, and ``ray.get`` + publish only those.

        ``ray.get`` is never issued on a ref that hasn't been reported ready
        by ``ray.wait(fetch_local=True)``: a ref stuck on a bad/dead node
        would otherwise block the whole thread forever and starve every
        other operator's metadata. A stuck ref just stays in ``pending``
        until Ray resolves or fails it.
        """
        try:
            pending: List["ray.ObjectRef"] = []
            while True:
                # Block on the request queue only when there's nothing in
                # flight; otherwise poll it so pending refs keep making progress.
                if pending:
                    try:
                        item = self._request_q.get_nowait()
                    except queue_module.Empty:
                        item = ()
                else:
                    item = self._request_q.get()
                if item is None:
                    return
                pending.extend(item)
                # Coalesce any other already-queued batches.
                while True:
                    try:
                        nxt = self._request_q.get_nowait()
                    except queue_module.Empty:
                        break
                    if nxt is None:
                        return
                    pending.extend(nxt)

                if not pending:
                    continue
                ready, pending = ray.wait(
                    pending,
                    num_returns=len(pending),
                    timeout=_FETCH_WAIT_TIMEOUT_S,
                    fetch_local=True,
                )
                if ready:
                    self._fetch(ready)
        except Exception:
            logger.exception("MetadataPrefetcher background thread died unexpectedly.")

Comment on lines +116 to +122
def drain(self) -> None:
"""Emit every pair whose metadata is now available, in per-op append
order, then fire postponed done callbacks for fully-drained tasks.

Must be called on the executor thread.
"""
for fifo in self._fifos.values():

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

If the background prefetcher thread dies unexpectedly, the pipeline will hang silently. We should check if the thread is still alive in drain() and raise an error if it died.

Suggested change
def drain(self) -> None:
"""Emit every pair whose metadata is now available, in per-op append
order, then fire postponed done callbacks for fully-drained tasks.
Must be called on the executor thread.
"""
for fifo in self._fifos.values():
def drain(self) -> None:
"""Emit every pair whose metadata is now available, in per-op append
order, then fire postponed done callbacks for fully-drained tasks.
Must be called on the executor thread.
"""
if self._started and not self._thread.is_alive() and not self._stopped:
raise RuntimeError("Metadata prefetcher background thread died unexpectedly.")
for fifo in self._fifos.values():

Comment on lines +919 to +922
shared_cpu -= max(usage_cpu - reserved_cpu, 0.0)
shared_gpu -= max(usage_gpu - reserved_gpu, 0.0)
shared_osm -= max(usage_osm - reserved_osm, 0.0)
shared_mem -= max(usage_mem - reserved_mem, 0.0)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

To prevent any potential nan propagation when shared_cpu is inf, we should guard the subtraction of exceeded resources.

Suggested change
shared_cpu -= max(usage_cpu - reserved_cpu, 0.0)
shared_gpu -= max(usage_gpu - reserved_gpu, 0.0)
shared_osm -= max(usage_osm - reserved_osm, 0.0)
shared_mem -= max(usage_mem - reserved_mem, 0.0)
# How much of the reserved resources are exceeded.
# If exceeded, we need to subtract from the remaining shared resources.
if shared_cpu != float("inf"):
shared_cpu -= max(usage_cpu - reserved_cpu, 0.0)
if shared_gpu != float("inf"):
shared_gpu -= max(usage_gpu - reserved_gpu, 0.0)
if shared_osm != float("inf"):
shared_osm -= max(usage_osm - reserved_osm, 0.0)
if shared_mem != float("inf"):
shared_mem -= max(usage_mem - reserved_mem, 0.0)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant