Repository navigation
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
|
||
| /// Safe to call from any thread. | ||
| bool IsMovePull(const ObjectID &object_id) const override | ||
| ABSL_LOCKS_EXCLUDED(move_pull_mutex_); |
There was a problem hiding this comment.
Missing Doxygen on new APIs
Low Severity
New and updated C++ declarations such as MarkMovePull, IsMovePull, and GetDependencyIsMove do not use Doxygen /** ... */ comments with @brief and @param/@return. That violates the Doxygen documentation rule for C++ functions and classes.
Additional Locations (2)
Triggered by project rule: Bugbot Rules
Reviewed by Cursor Bugbot for commit 69d2a94. Configure here.
69d2a94 to
3303ac6
Compare
| ASSERT_FALSE(task_spec.ArgIsMove(0)); | ||
| ASSERT_TRUE(task_spec.ArgIsMove(1)); | ||
| ASSERT_FALSE(task_spec.ArgIsMove(2)); | ||
| // One entry per by-ref arg, in GetDependencies() order. |
There was a problem hiding this comment.
don't need this comment
| /// thread, so the IPC is not blocked behind in-progress work on the task | ||
| /// execution service). | ||
| /// | ||
| /// \param is_move Parallel to `args`: whether the task consumes the arg |
There was a problem hiding this comment.
can you plz add the other fields to make it look nicer thanks
| // Sent back to the core worker to notify the wait has completed. | ||
| task_id: string; | ||
| attempt_number: int; | ||
| // Parallel to object_ids: true if the task consumes the object (TaskArg.is_move). |
3303ac6 to
85674f6
Compare
85674f6 to
10b5bda
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 10b5bda. Configure here.
|
|
||
| /// Return, for each dependency in GetDependencies() order, whether this task | ||
| /// consumes it (TaskArg.is_move). | ||
| std::vector<bool> GetDependencyIsMove() const; |
There was a problem hiding this comment.
Missing Doxygen on new C++ APIs
Low Severity
New and modified C++ declarations are missing Doxygen /** ... */ comments with @brief and @param/@return. GetDependencyIsMove has no @return, MarkMovePull/IsMovePull on ObjectManager have no API comment, and BeginArgsFetch documents only is_move.
Additional Locations (2)
Triggered by project rule: Bugbot Rules
Reviewed by Cursor Bugbot for commit 10b5bda. Configure here.
Signed-off-by: kartica.modi <kartica.modi@anyscale.com>
10b5bda to
ab45efc
Compare


This is PR 4 of a series of PRs to implement move semantics for plasma objects in core.
The complete design doc is here: https://docs.google.com/document/d/1ZJhgRmwVyKfNCR21CWf6xMcc0B6HJwSPQsfebTRvnnE/edit?tab=t.0#heading=h.7wcod1f9afpg
After #66803, an actor task's spec says which of its by-ref args it consumes (
TaskArg.is_move). This PR carries that flag to the raylet of the node where the actor runs, and makes that raylet remember, for each such arg it has to pull, that the pull is a move. Nothing reads the record yet: the next PR uses it on the receive path to pin the pulled copy as the object's new primary copy and report that to the owner.Changes
TaskSpecification::GetDependencyIsMove(). Oneboolper entry ofGetDependencies(), in the same order.The flag travels with the arg wait.
BeginArgsFetchand theAsyncWaitForArgscallback take the vector;RayletIpcClient::WaitForActorCallArgssends it in a newis_move: [bool]field ofWaitForActorCallArgsRequest, parallel toobject_ids.The raylet records it when it starts a pull.
ProcessWaitForActorCallArgs→AsyncWait→LeaseDependencyManager::StartOrUpdateWaitRequest(worker, refs, is_move). In the branch that handles a non-local arg, right before the pull, a moved arg is recorded withObjectManager::MarkMovePull. If the object is not local but a pull for it already exists (aray.getin flight, or another worker'sray.wait), the arg is still recorded and no second pull is started. An arg that is already local is neither pulled nor recorded; a later PR handles that case.StartOrUpdateWaitRequestis also used for an ordinaryray.wait; that caller passes an emptyis_movevector, so a plainray.waitnever records anything.ObjectManagerkeeps the record. It is a set of object IDs. The set is written on the main thread (MarkMovePulladds,CancelPullremoves) and will be read on the RPC threads (IsMovePull, fromReceiveObjectChunk, once per chunk, in the next PR), so it is guarded by a mutex; reads take a shared lock.An object is removed from the set when its last pull ends, not its first (there could be multiple pulls for the same object).
PullManageralready tracks which pulls still need an object and tellsCancelPullwhich objects nobody needs any more; the record is erased only for those. So if two pulls want the same object (aray.getin flight plus the actor's wait, orc.remote(r, r)), ending one of them keeps the record.Tests
task_spec_test:GetDependencyIsMoveon theTestArgIsMovefixture.lease_dependency_manager_test: a moved, a plain and a local moved arg (one record, two pulls); a plainray.waitrecords nothing; a moved arg whose pull already exists from aray.getor from another worker's wait; the same moved arg passed twice starts one pull.object_manager_test: the record is idempotent, survives cancelling one of two pulls, and is dropped with the last one.python/ray/tests/test_consume_once.py(32 passed), and a two-node script covering a cross-node consume with a plain arg,c.remote(r, r), a same-node consume, an ownerray.wait(fetch_local=True)before the consume, and a plainray.waitinside an actor.