[Data] Fixing ReorderingBundleQueue handling of empty output sequences#60470
[Data] Fixing ReorderingBundleQueue handling of empty output sequences#60470alexeykudinkin merged 14 commits intomasterfrom
ReorderingBundleQueue handling of empty output sequences#60470Conversation
There was a problem hiding this comment.
Code Review
This pull request aims to fix an issue where _OrderedOutputQueue could get stuck by refactoring its state transition logic. My review identified a critical issue in the new implementation of _OrderedOutputQueue.has_next(). The updated logic can cause a crash by returning True when no output is available, which violates the has_next/get_next contract. I've provided a code suggestion to fix this bug while preserving the intended fix for the 'stuck' issue.
Signed-off-by: Alexey Kudinkin <ak@anyscale.com>
de2db59 to
b945852
Compare
_OrderedOutputQueue to avoid getting stuckReorderingBundleQueue to avoid getting stuck
ReorderingBundleQueue to avoid getting stuckReorderingBundleQueue
Signed-off-by: Alexey Kudinkin <ak@anyscale.com>
Signed-off-by: Alexey Kudinkin <ak@anyscale.com>
Signed-off-by: Alexey Kudinkin <ak@anyscale.com>
Signed-off-by: Alexey Kudinkin <ak@anyscale.com>
Signed-off-by: Alexey Kudinkin <ak@anyscale.com>
783825c to
28731ed
Compare
Tidying up Signed-off-by: Alexey Kudinkin <ak@anyscale.com>
Signed-off-by: Alexey Kudinkin <ak@anyscale.com>
Signed-off-by: Alexey Kudinkin <ak@anyscale.com>
ReorderingBundleQueueReorderingBundleQueue handling of empty output sequences
Signed-off-by: Alexey Kudinkin <ak@anyscale.com>
|
|
||
| @override | ||
| def peek_next(self) -> Optional[RefBundle]: | ||
| return ( |
There was a problem hiding this comment.
do we also need to rotate the pointer here too?
| # - `has_next` would return False, (_inner[_current_key] is empty) | ||
| # - `get_next` will never be invoked (b/c `has_next` returns false) | ||
| # - `finalize(key=1)` has already been invoked, no pointer advancement will happen |
There was a problem hiding this comment.
are these comments and the above needed? I think they might be too implementation-detailed specific
There was a problem hiding this comment.
- This is a test so extra verbosity is fine
- Wanted to provide ample comment to explain how it would get stuck
Signed-off-by: Alexey Kudinkin <ak@anyscale.com>
Signed-off-by: Alexey Kudinkin <ak@anyscale.com>
| assert queue.peek_next() is bundle2 | ||
| assert queue.get_next() is bundle2 |
There was a problem hiding this comment.
why not always go into this case? (since target_op=="peek" also covers the "get" case)
There was a problem hiding this comment.
B/c we want to check that get also does advance
…ces (ray-project#60470) ## Description This PR revisits `ReorderingBundleQueue` to move pointer advancements from `get_next_inner` and `finalize` into `has_next` method to guarantee that the queue will not get stuck with any operations sequence. Currently, `ReorderingBundleQueue` could still get stuck in case of the sequence captured in `test_ordered_queue_getting_stuck`. The queue is guaranteed to traverse through all bundles so long as all keys are finalized (ie tasks finished). ## Related issues > Link related issues: "Fixes ray-project#1234", "Closes ray-project#1234", or "Related to ray-project#1234". ## Additional information > Optional: Add implementation details, API changes, usage examples, screenshots, etc. --------- Signed-off-by: Alexey Kudinkin <ak@anyscale.com> Signed-off-by: jinbum-kim <jinbum9958@gmail.com>
…ces (ray-project#60470) ## Description This PR revisits `ReorderingBundleQueue` to move pointer advancements from `get_next_inner` and `finalize` into `has_next` method to guarantee that the queue will not get stuck with any operations sequence. Currently, `ReorderingBundleQueue` could still get stuck in case of the sequence captured in `test_ordered_queue_getting_stuck`. The queue is guaranteed to traverse through all bundles so long as all keys are finalized (ie tasks finished). ## Related issues > Link related issues: "Fixes ray-project#1234", "Closes ray-project#1234", or "Related to ray-project#1234". ## Additional information > Optional: Add implementation details, API changes, usage examples, screenshots, etc. --------- Signed-off-by: Alexey Kudinkin <ak@anyscale.com>
…ces (ray-project#60470) ## Description This PR revisits `ReorderingBundleQueue` to move pointer advancements from `get_next_inner` and `finalize` into `has_next` method to guarantee that the queue will not get stuck with any operations sequence. Currently, `ReorderingBundleQueue` could still get stuck in case of the sequence captured in `test_ordered_queue_getting_stuck`. The queue is guaranteed to traverse through all bundles so long as all keys are finalized (ie tasks finished). ## Related issues > Link related issues: "Fixes ray-project#1234", "Closes ray-project#1234", or "Related to ray-project#1234". ## Additional information > Optional: Add implementation details, API changes, usage examples, screenshots, etc. --------- Signed-off-by: Alexey Kudinkin <ak@anyscale.com> Signed-off-by: 400Ping <jiekaichang@apache.org>
…ces (ray-project#60470) ## Description This PR revisits `ReorderingBundleQueue` to move pointer advancements from `get_next_inner` and `finalize` into `has_next` method to guarantee that the queue will not get stuck with any operations sequence. Currently, `ReorderingBundleQueue` could still get stuck in case of the sequence captured in `test_ordered_queue_getting_stuck`. The queue is guaranteed to traverse through all bundles so long as all keys are finalized (ie tasks finished). ## Related issues > Link related issues: "Fixes ray-project#1234", "Closes ray-project#1234", or "Related to ray-project#1234". ## Additional information > Optional: Add implementation details, API changes, usage examples, screenshots, etc. --------- Signed-off-by: Alexey Kudinkin <ak@anyscale.com> Signed-off-by: Adel Nour <ans9868@nyu.edu>
…ces (ray-project#60470) ## Description This PR revisits `ReorderingBundleQueue` to move pointer advancements from `get_next_inner` and `finalize` into `has_next` method to guarantee that the queue will not get stuck with any operations sequence. Currently, `ReorderingBundleQueue` could still get stuck in case of the sequence captured in `test_ordered_queue_getting_stuck`. The queue is guaranteed to traverse through all bundles so long as all keys are finalized (ie tasks finished). ## Related issues > Link related issues: "Fixes ray-project#1234", "Closes ray-project#1234", or "Related to ray-project#1234". ## Additional information > Optional: Add implementation details, API changes, usage examples, screenshots, etc. --------- Signed-off-by: Alexey Kudinkin <ak@anyscale.com> Signed-off-by: peterxcli <peterxcli@gmail.com>
…ces (ray-project#60470) ## Description This PR revisits `ReorderingBundleQueue` to move pointer advancements from `get_next_inner` and `finalize` into `has_next` method to guarantee that the queue will not get stuck with any operations sequence. Currently, `ReorderingBundleQueue` could still get stuck in case of the sequence captured in `test_ordered_queue_getting_stuck`. The queue is guaranteed to traverse through all bundles so long as all keys are finalized (ie tasks finished). ## Related issues > Link related issues: "Fixes ray-project#1234", "Closes ray-project#1234", or "Related to ray-project#1234". ## Additional information > Optional: Add implementation details, API changes, usage examples, screenshots, etc. --------- Signed-off-by: Alexey Kudinkin <ak@anyscale.com> Signed-off-by: peterxcli <peterxcli@gmail.com>
Description
This PR revisits
ReorderingBundleQueueto move pointer advancements fromget_next_innerandfinalizeintohas_nextmethod to guarantee that the queue will not get stuck with any operations sequence.Currently,
ReorderingBundleQueuecould still get stuck in case of the sequence captured intest_ordered_queue_getting_stuck.The queue is guaranteed to traverse through all bundles so long as all keys are finalized (ie tasks finished).
Related issues
Additional information