Skip to content

[core][move semantics][3/n]Populate task args with is_move for movable refs - #66803

Open
karticam wants to merge 2 commits into
karticam/move-sem-consume-and-enforcementsfrom
karticam/is-move-in-task-args
Open

karticam wants to merge 2 commits into
karticam/move-sem-consume-and-enforcementsfrom
karticam/is-move-in-task-args

Conversation

@karticam

@karticam karticam commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

This is PR 3 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

This marks each consume-once argument of an actor task with a new TaskArg.is_move flag in the task spec. After #66767, passing a consume-once ref to an actor task changes its state to MOVED; this PR records the same fact in the task spec, so that the consumer's raylet can later pull the object as a move instead of a copy. Nothing reads the flag yet: the consumer side comes in the next PRs.

Changes

  1. New proto field TaskArg.is_move. True when the task consumes the argument, which was created with _consume_once=True. Only set on pass-by-ref arguments. Arguments that aren't consume-once add nothing on the wire.
  2. ReferenceCounter::TryCommitMoves returns which args it moved. Its return type changes from Status to StatusOr<std::vector<bool>>: on success, one entry per arg ID, true if this call moved the object (it was MOVABLE). The entries are filled in the validation pass, so both entries of a repeated arg (c.remote(r, r), allowed by [core][move semantics][2/n] Change state to moved on consumption and add enforcements #66767) are true. NOT_MOVABLE args are false. On rejection (an arg already MOVED) it returns the same InvalidArgument as before.
  3. SubmitActorTask sets the flag. The commit loop also records each by-ref arg's index. After the task spec is built, the moved indices get TaskSpecBuilder::SetArgIsMove(i) (new). The proto arg order is the args order, so the indices line up.
  4. New accessor TaskSpecification::ArgIsMove(i), for the consumer side in the next PRs.
  5. The flag is cleared when the arg is inlined. A small consume-once object lives in the caller's in-memory store, and the dependency resolver copies its value into the task spec (InlineDependencies). There's nothing to pull or pin, so InlineDependencies clears is_move. An object in plasma stays by-ref and keeps the flag. (Tensor-transport args, which keep their object_ref when inlined, can't be consume-once: [core][move semantics][1/n] Adding consume_once flag in actor methods #66620 rejects _consume_once with tensor_transport.)
  6. Task retries reuse the stored task spec, so a retried task keeps is_move.

Stack created with GitHub Stacks CLI • Give Feedback 💬

@karticam
karticam added this pull request to stack #66768 October 7, 2026 02:02
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@karticam
karticam force-pushed the karticam/is-move-in-task-args branch from 6b6c4f0 to 6ec8cbb Compare October 7, 2026 02:02
@karticam
karticam marked this pull request as ready for review October 8, 2026 20:33
@karticam
karticam requested a review from a team as a code owner October 8, 2026 20:33

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ 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 913a17f. Configure here.

Comment thread src/ray/protobuf/common.proto Resolved

/// Return true if this task consumes the argument at the given index, which was
/// created with consume_once.
bool ArgIsMove(size_t arg_index) const;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

New APIs missing Doxygen tags

Low Severity

⚠️ Document functions and classes with Doxygen /** ... */ block comments using @ tags (@brief, and @param/@return where applicable). ArgIsMove and SetArgIsMove have brief comments but omit @param/@return for arg_index and the return value.

Additional Locations (1)
Fix in Cursor Fix in Web

Triggered by project rule: Bugbot Rules

Reviewed by Cursor Bugbot for commit 913a17f. Configure here.

@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 tracking for arguments that are consumed (moved) by a task (created with consume_once). It adds an is_move field to the TaskArg protobuf message, updates ReferenceCounter::TryCommitMoves to return a list of booleans indicating which arguments were moved, and ensures that is_move is cleared if an argument is inlined during dependency resolution. It also includes corresponding unit tests and updates to task building and submission. I have no feedback to provide as there are no review comments.

@ray-gardener ray-gardener Bot added the core Issues that should be addressed in Ray Core label Oct 9, 2026
@karticam karticam added go add ONLY when ready to merge, run all tests unstale A PR that has been marked unstale. It will not get marked stale again if this label is on it. labels Oct 16, 2026
@karticam
karticam force-pushed the karticam/is-move-in-task-args branch from 913a17f to d4703ac Compare October 21, 2026 05:14
ASSERT_TRUE(dependencies_resolved.get_future().get());
ASSERT_TRUE(ok);
ASSERT_TRUE(task.ArgByRef(0));
// An argument that stays in plasma keeps is_move.

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.

remove comment

ASSERT_FALSE(task.ArgByRef(1));
ASSERT_NE(task.ArgData(0), nullptr);
ASSERT_NE(task.ArgData(1), nullptr);
// An inlined argument is not moved.

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.

remove comment

}

mutable_arg->set_is_inlined(true);
// The value is now in the task spec (inlined), so there is nothing to move.

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.

We should document this, for in-lined objects it's a no-op

move_state);
return id;
};
ObjectID plain = add_owned_object(MoveState::NOT_MOVABLE);

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.

can you rename plain to not_movable


// A repeated ID is moved once; NOT_MOVABLE and unknown IDs are left alone.
ASSERT_TRUE(rc->TryCommitMoves({plain, movable_1, movable_1, unknown}).ok());
// A repeated ID is moved once and both entries are reported as moves; NOT_MOVABLE

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.

can you clarify the state machine for MOVABLE here, would help

@karticam
karticam force-pushed the karticam/is-move-in-task-args branch 2 times, most recently from cbe3351 to 95c6f8b Compare November 3, 2026 03:07
Signed-off-by: kartica.modi <kartica.modi@anyscale.com>
Signed-off-by: kartica.modi <kartica.modi@anyscale.com>
@karticam
karticam force-pushed the karticam/is-move-in-task-args branch from 95c6f8b to 2ae4e61 Compare November 14, 2026 02:36

This branch has not been deployed

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

Labels

core Issues that should be addressed in Ray Core go add ONLY when ready to merge, run all tests unstale A PR that has been marked unstale. It will not get marked stale again if this label is on it.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants