Skip to content

[WIP]Support nixl PayloadTransfer for SimpleStorage - #214

Open
shun001 wants to merge 1 commit into
Ascend:mainfrom
shun001:208-nixl
Open

shun001 wants to merge 1 commit into
Ascend:mainfrom
shun001:208-nixl

Conversation

@shun001

@shun001 shun001 commented Nov 11, 2026

Copy link
Copy Markdown

Summary

This PR adds an opt-in NIXL payload backend for SimpleStorage. It introduces a common PayloadTransfer interface so the Manager and StorageUnit can use either ZMQ multipart messages or NIXL notifications with UCX READ for PUT, GET, and UPDATE. The design follows the interface proposed in [RFC #208](#208).

Design and implementation

  • Common storage handling. The Manager submits a request header and ordered byte spans through PayloadTransfer. Both backends invoke the same StorageUnit handler for parsing, validation, reads, writes, and merges.
  • Receiver-initiated transfers. The NIXL backend uses notifications to exchange request headers, buffer descriptors, and replies. For PUT and UPDATE, the Manager publishes the input buffer and the StorageUnit reads it before applying the operation. For GET, the StorageUnit publishes the encoded result and the Manager reads it.
  • Buffer lifetime and write completion. The sender keeps its published buffer registered until it receives confirmation that the receiver has finished accessing it. PUT and UPDATE replies report storage acceptance separately from buffer release. If a write reply is lost, the Manager reports an unknown write outcome and avoids replaying the operation.
  • Backend selection. ZMQ is the default payload backend and also serves administrative requests such as CLEAR and checkpoint operations. NIXL endpoints are distributed through the existing TQ bootstrap process.

To enable the NIXL backend:

backend:
  SimpleStorage:
    payload_transfer:
      backend: nixl-ucx

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-11T07:12:10.015103Z d394bf9 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d394bf99bd

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

raise TransferTimeout("NIXL storage request timed out", wire_bytes=wire_bytes)
await asyncio.sleep(0.001)
response = reply.result()
except Exception as exc:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Treat cancelled writes as outcome-unknown after sending

When a PUT/UPDATE task is cancelled after send_notification but before the reply, asyncio.CancelledError inherits BaseException and therefore bypasses this except Exception; the storage unit may still apply the write while the caller sees only cancellation. A caller using asyncio.wait_for and retrying can consequently apply a non-idempotent UPDATE twice, so this post-send cancellation path needs to finish determining the outcome or surface WriteOutcomeUnknown just like the timeout path.

Useful? React with 👍 / 👎.

logger.exception("NIXL storage request %s failed", request_id)
reply["error"] = str(exc)[:4096]
try:
packed = self._pack_control(reply)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Prevent oversized update acknowledgements after committing

When an UPDATE produces enough per-sample stored_shapes to make its reply exceed MAX_CONTROL_BYTES (1 MiB), _handle has already called apply_update before this packing attempt. _pack_control then falls into the outcome-unknown reply, so AsyncSimpleStorageManager.update_data raises before notify_data_update; for a newly added or shape-changing field, storage is committed while controller metadata remains stale. Large valid updates therefore deterministically leave the planes inconsistent; reject or preflight before applying, or carry the acknowledgement body outside the capped notification.

Useful? React with 👍 / 👎.

@ji-huazhong

Copy link
Copy Markdown
Collaborator

cc @Ginray

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.

2 participants