feat: Connect Library #1478
feat: Connect Library #1478
Conversation
whoisj
requested review from
piotrm-nvidia,
ptarasiewiczNV and
ryanolson
as code owners
whoisj
requested review from
grahamking,
paulhendricks and
biswapanda
as code owners
whoisj
requested review from
tmonty12,
GuanLuo,
rmccorm4,
tedzhouhk,
alec-flowers,
kkranen,
jthomson04,
ishandhanani,
PeaBrane and
a team
as code owners
|
👋 Hi whoisj! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
bot
added
external-contribution
whoisj
removed
the
external-contribution
WalkthroughThis change introduces comprehensive documentation and a new Python module for the Changes
Sequence Diagram(s)sequenceDiagram
participant LocalWorker
participant RemoteWorker
participant NIXL
LocalWorker->>NIXL: Register memory (Descriptor)
RemoteWorker->>NIXL: Register memory (Descriptor)
LocalWorker->>LocalWorker: Create ReadableOperation/WritableOperation
LocalWorker->>RemoteWorker: Send RDMA metadata (via HTTP/TCP+NATS)
RemoteWorker->>NIXL: Begin ReadOperation/WriteOperation with metadata
NIXL-->>RemoteWorker: Data transfer (RDMA)
RemoteWorker-->>LocalWorker: Notify completion (via await/wait)
Possibly related PRs
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (21)
components/connect/device_kind.md (1)
36-43: Fix Markdown list indentation
The items under Related Classes are indented by two spaces, which conflicts with markdownlint’s expected left-aligned list. Remove the extra spaces so list markers start at column 1.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
36-36: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
37-37: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
38-38: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
39-39: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
40-40: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
41-41: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
42-42: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
43-43: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
components/connect/device.md (2)
24-24: Add missing comma for clarity
Consider inserting a comma after"cuda:1"in the example sentence to improve readability:“…to reference the second GPU in a system,
\"cuda:1\", can be used.”🧰 Tools
🪛 LanguageTool
[uncategorized] ~24-~24: Possible missing comma found.
Context: ...ample, to reference the second GPU in a system"cuda:1"can be used. By default, wh...(AI_HYDRA_LEO_MISSING_COMMA)
46-53: Fix Markdown list indentation
The Related Classes list items are indented by two spaces. Align all list markers at the left margin to satisfy markdownlint rules.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
46-46: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
47-47: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
48-48: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
49-49: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
50-50: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
51-51: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
52-52: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
53-53: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
components/connect/descriptor.md (1)
38-45: Fix Markdown list indentation
Remove the leading spaces from the Related Classes list items so they start at column 1, aligning with markdownlint expectations.components/connect/operation_status.md (1)
52-59: Fix Markdown list indentation
The Related Classes list items are indented. Align them to the left margin (no leading spaces) to conform with markdownlint.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
52-52: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
53-53: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
54-54: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
55-55: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
56-56: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
57-57: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
58-58: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
59-59: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
components/connect/rdma_metadata.md (2)
20-20: Improve readability with comma
Add a comma after “type” in the opening description:“A Pydantic type, intended to provide JSON-serialized RDMA metadata…”
🧰 Tools
🪛 LanguageTool
[uncategorized] ~20-~20: Possible missing comma found.
Context: ...dynamo.connect.RdmaMetadata A Pydantic type intended to provide JSON serialized RDM...(AI_HYDRA_LEO_MISSING_COMMA)
39-46: Fix Markdown list indentation
The Related Classes items are indented by two spaces. Remove indentation so all list markers start at the left margin.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
39-39: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
40-40: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
41-41: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
42-42: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
43-43: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
44-44: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
45-45: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
46-46: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
components/connect/readable_operation.md (1)
69-76: Fix unordered list indentation.The markdown linter indicates that the list items have incorrect indentation. They should start at column 0 instead of having 2-space indentation.
Apply this diff to fix the list indentation:
- - [Connector](connector.md) - - [Descriptor](descriptor.md) - - [Device](device.md) - - [OperationStatus](operation_status.md) - - [RdmaMetadata](rdma_metadata.md) - - [ReadOperation](read_operation.md) - - [WritableOperation](writable_operation.md) - - [WriteOperation](write_operation.md) +- [Connector](connector.md) +- [Descriptor](descriptor.md) +- [Device](device.md) +- [OperationStatus](operation_status.md) +- [RdmaMetadata](rdma_metadata.md) +- [ReadOperation](read_operation.md) +- [WritableOperation](writable_operation.md) +- [WriteOperation](write_operation.md)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
69-69: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
70-70: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
71-71: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
72-72: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
73-73: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
74-74: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
75-75: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
76-76: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
components/connect/writable_operation.md (1)
27-28: Fix grammatical error with "therefore" usage.The word "therefore" should be preceded by a semicolon or period when used as a conjunction, and "awaited" should be "waited" in this context.
Apply this diff to fix the grammatical error:
-Disposal of the operation's object reference will instruct the RDMA subsystem to cancel the write operation, -therefore the operation should be awaited until complete unless cancellation is intended. +Disposal of the operation's object reference will instruct the RDMA subsystem to cancel the write operation; +therefore, the operation should be waited for until complete unless cancellation is intended.🧰 Tools
🪛 LanguageTool
[typographical] ~27-~27: The word “therefore” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ... the RDMA subsystem to cancel the write operation, therefore the operation should be awaited until c...(THUS_SENTENCE)
[misspelling] ~28-~28: Did you mean “waited”?
Context: ...tion, therefore the operation should be awaited until complete unless cancellation is i...(CONFUSION_AWAIT_WAIT)
components/connect/write_operation.md (3)
27-27: Fix typo in "operation's".There's a possessive apostrophe error in "operations's" which should be "operation's".
Apply this diff to fix the typo:
-Disposal of the operations's object reference will instruct the RDMA subsystem to cancel the write operation, +Disposal of the operation's object reference will instruct the RDMA subsystem to cancel the write operation,🧰 Tools
🪛 LanguageTool
[typographical] ~27-~27: The word “therefore” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ... the RDMA subsystem to cancel the write operation, therefore the operation should be awaited until c...(THUS_SENTENCE)
27-28: Fix grammatical error with "therefore" usage.The word "therefore" should be preceded by a semicolon or period when used as a conjunction, and "awaited" should be "waited" in this context.
Apply this diff to fix the grammatical error:
-therefore the operation should be awaited until complete unless cancellation is intended. +therefore, the operation should be waited for until complete unless cancellation is intended.🧰 Tools
🪛 LanguageTool
[typographical] ~27-~27: The word “therefore” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ... the RDMA subsystem to cancel the write operation, therefore the operation should be awaited until c...(THUS_SENTENCE)
[misspelling] ~28-~28: Did you mean “waited”?
Context: ...tion, therefore the operation should be awaited until complete unless cancellation is i...(CONFUSION_AWAIT_WAIT)
66-73: Fix unordered list indentation.The markdown linter indicates that the list items have incorrect indentation. They should start at column 0 instead of having 2-space indentation.
Apply this diff to fix the list indentation:
- - [Connector](connector.md) - - [Descriptor](descriptor.md) - - [Device](device.md) - - [OperationStatus](operation_status.md) - - [RdmaMetadata](rdma_metadata.md) - - [ReadOperation](read_operation.md) - - [ReadableOperation](readable_operation.md) - - [WritableOperation](writable_operation.md) +- [Connector](connector.md) +- [Descriptor](descriptor.md) +- [Device](device.md) +- [OperationStatus](operation_status.md) +- [RdmaMetadata](rdma_metadata.md) +- [ReadOperation](read_operation.md) +- [ReadableOperation](readable_operation.md) +- [WritableOperation](writable_operation.md)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
66-66: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
67-67: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
68-68: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
69-69: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
70-70: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
71-71: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
72-72: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
73-73: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
components/connect/README.md (2)
21-21: Fix missing verb in the sentence.The sentence is missing the verb "is" to be grammatically correct.
Apply this diff to fix the grammar:
-The primary goal of this library to simplify the integration of NIXL based RDMA into inference applications. +The primary goal of this library is to simplify the integration of NIXL based RDMA into inference applications.
76-84: Consider improving readability by adding articles for consistency.While not grammatically incorrect, adding "The" before worker names would improve readability and consistency with professional documentation standards.
Apply this diff to improve readability:
- 3. Decode Worker then requests a Prefill Worker to provide key-value data for the LLM powering the Decode Worker. + 3. The Decode Worker then requests a Prefill Worker to provide key-value data for the LLM powering the Decode Worker. - 4. Prefill Worker then requests that the image be processed and provided as embeddings by the Encode Worker. + 4. The Prefill Worker then requests that the image be processed and provided as embeddings by the Encode Worker.🧰 Tools
🪛 LanguageTool
[uncategorized] ~76-~76: You might be missing the article “the” here.
Context: ...the first available Decode Worker. 3. Decode Worker then requests a Prefill Worker t...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~78-~78: You might be missing the article “the” here.
Context: ...he LLM powering the Decode Worker. 4. Prefill Worker then requests that the image be ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
components/connect/connector.md (2)
34-34: Consider using "before" instead of "prior to" for conciseness.The phrase "prior to" is wordy and can be simplified.
-provides RDMA metadata ([RdmaMetadata](rdma_metadata.md)) via its `.metadata()` method, functionality to query the operation's current state, as well as the ability to cancel the operation prior to its completion. +provides RDMA metadata ([RdmaMetadata](rdma_metadata.md)) via its `.metadata()` method, functionality to query the operation's current state, as well as the ability to cancel the operation before its completion.🧰 Tools
🪛 LanguageTool
[style] ~34-~34: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ... as the ability to cancel the operation prior to its completion. The RDMA metadata must...(EN_WORDINESS_PREMIUM_PRIOR_TO)
74-75: Fix grammar: Add semicolon before "therefore".The word "therefore" is an adverb that should be separated from the preceding clause with a semicolon.
For line 74-75:
-Disposal of the object will instruct the RDMA subsystem to cancel the operation, -therefore the operation should be awaited until completed unless cancellation is intended. +Disposal of the object will instruct the RDMA subsystem to cancel the operation; +therefore, the operation should be awaited until completed unless cancellation is intended.Apply the same fix to lines 90-91, 104-105, and 118-119.
Also applies to: 90-91, 104-105, 118-119
🧰 Tools
🪛 LanguageTool
[typographical] ~74-~74: The word “therefore” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...struct the RDMA subsystem to cancel the operation, therefore the operation should be awaited until c...(THUS_SENTENCE)
[misspelling] ~75-~75: Did you mean “waited”?
Context: ...tion, therefore the operation should be awaited until completed unless cancellation is ...(CONFUSION_AWAIT_WAIT)
components/connect/src/dynamo/connect/__init__.py (5)
223-226: Simplify conditional by removing unnecessary "elif".After a raise statement, the subsequent condition doesn't need to be an "elif".
- if (isinstance(local_descriptors, list) and isinstance(remote_descriptors, list) and len(local_descriptors) != len(remote_descriptors)): - raise ValueError("When `local_descriptors` and `remote_descriptors` are lists, they must have the same length.") - elif isinstance(local_descriptors, list) != isinstance(remote_descriptors, list): - raise ValueError("Both `local_descriptors` and `remote_descriptors` must be either lists or single descriptors.") + if (isinstance(local_descriptors, list) and isinstance(remote_descriptors, list) and len(local_descriptors) != len(remote_descriptors)): + raise ValueError("When `local_descriptors` and `remote_descriptors` are lists, they must have the same length.") + if isinstance(local_descriptors, list) != isinstance(remote_descriptors, list): + raise ValueError("Both `local_descriptors` and `remote_descriptors` must be either lists or single descriptors.")🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 223-226: Unnecessary "elif" after "raise", remove the leading "el" from "elif"
(R1720)
429-433: Fix typos in docstrings and comments.Several typos need to be corrected:
- Line 431: "iis provied" → "is provided"
- Line 433: "provied" → "provided"
- Line 476: "avalailable" → "available"
- TypeError - When `runtime` iis provied and not of type `dynamo.runtime.DistributedRuntime`. - TypeError - When `worker_id` is provied and not of type `uuid.UUID`. + TypeError + When `runtime` is provided and not of type `dynamo.runtime.DistributedRuntime`. + TypeError + When `worker_id` is provided and not of type `uuid.UUID`.- # Note: cuda.is_avalailable initializes cuda + # Note: cuda.is_available initializes cudaAlso applies to: 476-476
739-739: Add missing space in comparison operator.- self._data_ref = data[3] if len(data) >=4 else None + self._data_ref = data[3] if len(data) >= 4 else None
915-920: Simplify enum str methods by removing unnecessary "elif".For DeviceKind (lines 915-920):
def __str__(self) -> str: if self == DeviceKind.HOST: return "cpu" - elif self == DeviceKind.CUDA: + if self == DeviceKind.CUDA: return "cuda" - else: - return "<invalid>" + return "<invalid>"For OperationKind (lines 933-938):
def __str__(self) -> str: if self == OperationKind.READ: return "READ" - elif self == OperationKind.WRITE: + if self == OperationKind.WRITE: return "WRITE" - else: - return "<invalid>" + return "<invalid>"Also applies to: 933-938
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 915-920: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
965-976: Simplify OperationStatus str method.def __str__(self) -> str: if self == OperationStatus.INITIALIZED: return "INIT" - elif self == OperationStatus.IN_PROGRESS: + if self == OperationStatus.IN_PROGRESS: return "PROC" - elif self == OperationStatus.COMPLETE: + if self == OperationStatus.COMPLETE: return "DONE" - elif self == OperationStatus.ERRORED: + if self == OperationStatus.ERRORED: return "ERR" - elif self == OperationStatus.CANCELLED: + if self == OperationStatus.CANCELLED: return "STOP" - else: - return "<invalid>" + return "<invalid>"🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 965-976: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
components/connect/README.md(1 hunks)components/connect/connector.md(1 hunks)components/connect/descriptor.md(1 hunks)components/connect/device.md(1 hunks)components/connect/device_kind.md(1 hunks)components/connect/operation_status.md(1 hunks)components/connect/rdma_metadata.md(1 hunks)components/connect/read_operation.md(1 hunks)components/connect/readable_operation.md(1 hunks)components/connect/src/dynamo/connect/__init__.py(1 hunks)components/connect/writable_operation.md(1 hunks)components/connect/write_operation.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
components/connect/device_kind.md
36-36: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
37-37: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
38-38: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
39-39: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
40-40: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
41-41: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
42-42: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
43-43: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
components/connect/operation_status.md
52-52: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
53-53: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
54-54: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
55-55: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
56-56: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
57-57: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
58-58: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
59-59: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
components/connect/rdma_metadata.md
39-39: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
40-40: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
41-41: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
42-42: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
43-43: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
44-44: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
45-45: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
46-46: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
components/connect/write_operation.md
66-66: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
67-67: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
68-68: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
69-69: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
70-70: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
71-71: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
72-72: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
73-73: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
components/connect/device.md
46-46: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
47-47: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
48-48: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
49-49: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
50-50: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
51-51: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
52-52: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
53-53: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
components/connect/readable_operation.md
69-69: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
70-70: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
71-71: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
72-72: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
73-73: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
74-74: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
75-75: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
76-76: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🪛 LanguageTool
components/connect/README.md
[uncategorized] ~23-~23: A punctuation mark might be missing here.
Context: ...sing the Connect library begin with the Connector class and the ...
(AI_EN_LECTOR_MISSING_PUNCTUATION)
[style] ~30-~30: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...le a remote worker to read from. 2. Register local writable memory: Register ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~44-~44: Did you mean “it's” (contraction of ‘it is’)?
Context: ...write)-able operation" and the other is its correctly paired "(read|write) operatio...
(ITS_PREMIUM)
[uncategorized] ~76-~76: You might be missing the article “the” here.
Context: ...the first available Decode Worker. 3. Decode Worker then requests a Prefill Worker t...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~78-~78: You might be missing the article “the” here.
Context: ...he LLM powering the Decode Worker. 4. Prefill Worker then requests that the image be ...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
components/connect/writable_operation.md
[typographical] ~27-~27: The word “therefore” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ... the RDMA subsystem to cancel the write operation, therefore the operation should be awaited until c...
(THUS_SENTENCE)
[misspelling] ~28-~28: Did you mean “waited”?
Context: ...tion, therefore the operation should be awaited until complete unless cancellation is i...
(CONFUSION_AWAIT_WAIT)
components/connect/rdma_metadata.md
[uncategorized] ~20-~20: Possible missing comma found.
Context: ...dynamo.connect.RdmaMetadata A Pydantic type intended to provide JSON serialized RDM...
(AI_HYDRA_LEO_MISSING_COMMA)
components/connect/read_operation.md
[typographical] ~27-~27: The word “therefore” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...t the RDMA subsystem to cancel the read operation, therefore the operation should be awaited until c...
(THUS_SENTENCE)
[misspelling] ~28-~28: Did you mean “waited”?
Context: ...tion, therefore the operation should be awaited until complete or and deleted prior to ...
(CONFUSION_AWAIT_WAIT)
[style] ~28-~28: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...e awaited until complete or and deleted prior to completion when cancellation is intende...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
components/connect/write_operation.md
[typographical] ~27-~27: The word “therefore” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ... the RDMA subsystem to cancel the write operation, therefore the operation should be awaited until c...
(THUS_SENTENCE)
[misspelling] ~28-~28: Did you mean “waited”?
Context: ...tion, therefore the operation should be awaited until complete unless cancellation is i...
(CONFUSION_AWAIT_WAIT)
components/connect/device.md
[uncategorized] ~24-~24: Possible missing comma found.
Context: ...ample, to reference the second GPU in a system "cuda:1" can be used. By default, wh...
(AI_HYDRA_LEO_MISSING_COMMA)
components/connect/readable_operation.md
[typographical] ~27-~27: The word “therefore” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...struct the RDMA subsystem to cancel the operation, therefore the operation should be awaited until c...
(THUS_SENTENCE)
[misspelling] ~28-~28: Did you mean “waited”?
Context: ...tion, therefore the operation should be awaited until complete or and deleted prior to ...
(CONFUSION_AWAIT_WAIT)
[style] ~28-~28: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...e awaited until complete or and deleted prior to completion when cancellation is intende...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
components/connect/connector.md
[style] ~34-~34: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ... as the ability to cancel the operation prior to its completion. The RDMA metadata must...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[typographical] ~74-~74: The word “therefore” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...struct the RDMA subsystem to cancel the operation, therefore the operation should be awaited until c...
(THUS_SENTENCE)
[misspelling] ~75-~75: Did you mean “waited”?
Context: ...tion, therefore the operation should be awaited until completed unless cancellation is ...
(CONFUSION_AWAIT_WAIT)
[typographical] ~90-~90: The word “therefore” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...struct the RDMA subsystem to cancel the operation, therefore the operation should be awaited until c...
(THUS_SENTENCE)
[misspelling] ~91-~91: Did you mean “waited”?
Context: ...tion, therefore the operation should be awaited until completed unless cancellation is ...
(CONFUSION_AWAIT_WAIT)
[typographical] ~104-~104: The word “therefore” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...struct the RDMA subsystem to cancel the operation, therefore the operation should be awaited until c...
(THUS_SENTENCE)
[misspelling] ~105-~105: Did you mean “waited”?
Context: ...tion, therefore the operation should be awaited until completed unless cancellation is ...
(CONFUSION_AWAIT_WAIT)
[typographical] ~118-~118: The word “therefore” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...struct the RDMA subsystem to cancel the operation, therefore the operation should be awaited until c...
(THUS_SENTENCE)
[misspelling] ~119-~119: Did you mean “waited”?
Context: ...tion, therefore the operation should be awaited until completed unless cancellation is ...
(CONFUSION_AWAIT_WAIT)
🪛 Pylint (3.3.7)
components/connect/src/dynamo/connect/__init__.py
[refactor] 52-52: Too many instance attributes (9/7)
(R0902)
[refactor] 57-57: Too many arguments (6/5)
(R0913)
[refactor] 57-57: Too many positional arguments (6/5)
(R0917)
[refactor] 192-192: Too many arguments (6/5)
(R0913)
[refactor] 192-192: Too many positional arguments (6/5)
(R0917)
[refactor] 223-226: Unnecessary "elif" after "raise", remove the leading "el" from "elif"
(R1720)
[refactor] 676-676: Consider merging these isinstance calls to isinstance(data, (bytes, torch.Tensor, tuple))
(R1701)
[refactor] 708-708: Consider merging these isinstance calls to isinstance(data[1], (Device, str))
(R1701)
[refactor] 732-732: Consider merging these isinstance calls to isinstance(data[2], (Device, str))
(R1701)
[refactor] 637-637: Too many branches (13/12)
(R0912)
[refactor] 915-920: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 933-938: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 965-976: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[error] 1251-1251: Method 'validate_operation_kind' should have "self" as first argument
(E0213)
[refactor] 1269-1269: Consider merging these isinstance calls to isinstance(nixl_metadata, (bytes, str))
(R1701)
[error] 1354-1354: Method 'validate_memtype' should have "self" as first argument
(E0213)
[error] 1363-1363: Method 'validate_ptr' should have "self" as first argument
(E0213)
[error] 1369-1369: Method 'validate_size' should have "self" as first argument
(E0213)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: pre-merge-rust (lib/runtime/examples)
- GitHub Check: pre-merge-rust (lib/bindings/python)
- GitHub Check: pre-merge-rust (.)
- GitHub Check: Build and Test - vllm
🔇 Additional comments (3)
components/connect/README.md (1)
136-136: Update class reference to reflect the renaming from SerializedRequest to RdmaMetadata.According to the PR objectives,
connect.SerializedRequestwas renamed toconnect.RdmaMetadata. The documentation should reflect this change.Apply this diff to update the class reference:
- - [SerializedRequest](serialized_request.md) + - [RdmaMetadata](rdma_metadata.md)Likely an incorrect or invalid review comment.
components/connect/src/dynamo/connect/__init__.py (2)
272-276: Document Python 3.10+ requirement for match/case statements.The code uses match/case statements which require Python 3.10 or higher. This should be documented in the project requirements.
Would you like me to check if the Python version requirement is documented elsewhere in the project?
Also applies to: 337-345
857-872:⚠️ Potential issueFix potential UnboundLocalError in Device initialization.
The
device_idvariable may not be assigned before use if the metadata string doesn't match expected patterns.elif metadata.startswith("cpu") or metadata.startswith("host"): kind = DeviceKind.HOST device_id = 0 else: + device_id = 0 # Default value before raising error raise ValueError("Argument `metadata` must be in the format 'cuda:<device_id>' or 'cpu'.")Likely an incorrect or invalid review comment.
whoisj
force-pushed
the
whoisj/connect/relocate
branch
2 times, most recently
from
b8434ee to
e810c92
Compare
|
@coderabbitai resolve |
✅ Actions performedComments resolved. |
This change delivers the Connect library. Connect (aka dynamo.connect) is a Pythonic wrapper of NIXL, with specialized Dynamo support aspects, which specialized in worker-to-worker data transfer (as opposed to KV cache data transfer). Includes README and API documentation. Renamed `connect.SerializedRequest` to `connect.RdmaMetadata` for type-purpose clarity reasons. This change will require updating the Multimodal Example prior to deleting the previous incarnation of the connect library.
whoisj
force-pushed
the
whoisj/connect/relocate
branch
from
e810c92 to
6fae4b5
Compare
This change delivers the Connect library.
Connect (aka dynamo.connect) is a Pythonic wrapper of NIXL, with specialized Dynamo support aspects, which specialized in worker-to-worker data transfer (as opposed to KV cache data transfer).
Includes README and API documentation.
Renamed
connect.SerializedRequesttoconnect.RdmaMetadatafor type-purpose clarity reasons. This change will require updating the Multimodal Example prior to deleting the previous incarnation of the connect library.DIS-539
Summary by CodeRabbit
New Features
Documentation