Rework documents for this project#118
Conversation
|
@archlitchi: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughREADME guidance was condensed and linked to new HAMi and Volcano deployment guides. The guides document deployment, soft slicing, scheduler integration, usage, and metrics. Chart-managed vNPU monitoring configuration was removed in favor of embedded metrics documentation. ChangesDocumentation and monitoring integration
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
README.md (1)
40-44: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the Buildx output explicit.
If this command is intended to create a local image, add
--load; with non-default builders, an unspecified output may remain only in the builder cache. Otherwise, document the intended--pushworkflow. (docs.docker.com)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 40 - 44, Update the README’s Build image command to make its Buildx output behavior explicit: add --load when the command is intended to create a local image, or document the intended --push workflow if the image should be published.docs/hami.md (1)
7-23: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCarry the ARM prerequisite into every standalone deployment guide. README documents that
hami-vnpu-corecurrently supports ARM only, but users following any dedicated guide can miss this deployment-blocking constraint.
docs/hami.md#L7-L23: mention the ARM-only limitation for soft slicing.docs/hami_cn.md#L7-L23: add the equivalent Chinese prerequisite.docs/volcano.md#L7-L18: mention the limitation before Volcano deployment.docs/volcano_cn.md#L7-L18: add the equivalent Chinese prerequisite.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/hami.md` around lines 7 - 23, Document the ARM-only support limitation for hami-vnpu-core soft slicing in the prerequisites of docs/hami.md lines 7-23, docs/hami_cn.md lines 7-23, docs/volcano.md lines 7-18, and docs/volcano_cn.md lines 7-18; add equivalent English and Chinese wording in each standalone deployment guide before its deployment instructions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/hami.md`:
- Around line 77-91: The soft-slicing Pod examples are invalid because
containers is nested under metadata.annotations and spec is missing. In
docs/hami.md lines 77-91 and docs/hami_cn.md lines 79-93, add spec at the Pod
level and move the containers definition beneath it with correct YAML
indentation, preserving the existing container and resource fields.
- Around line 77-91: Update the HAMi Ascend soft-slicing Pod examples to include
runtimeClassName: ascend under spec. Apply this to docs/hami.md lines 77-91 and
99-115, and docs/hami_cn.md lines 79-93 and 101-117, covering both English and
Chinese examples.
- Around line 41-49: Update the ConfigMap guidance in docs/hami.md (lines 41-49)
and docs/hami_cn.md (lines 41-53) to use the same conditional namespace flow:
skip applying the Ascend ConfigMap when HAMi and ascend-device-plugin share a
namespace, and place the apply command only under the different-namespace path.
Explicitly instruct users to preserve HAMi’s existing hami-scheduler-device
entries and merge only the vnpus section, with the same warning and
synchronization guidance in both languages.
In `@README.md`:
- Around line 11-15: Update the headings for “Template-based Hard Slicing
(vNPU)” and “Soft Slicing with Runtime Interception (hami-vnpu-core)” from level
four to level three so they directly follow the surrounding “Introduction”
level-two heading.
---
Nitpick comments:
In `@docs/hami.md`:
- Around line 7-23: Document the ARM-only support limitation for hami-vnpu-core
soft slicing in the prerequisites of docs/hami.md lines 7-23, docs/hami_cn.md
lines 7-23, docs/volcano.md lines 7-18, and docs/volcano_cn.md lines 7-18; add
equivalent English and Chinese wording in each standalone deployment guide
before its deployment instructions.
In `@README.md`:
- Around line 40-44: Update the README’s Build image command to make its Buildx
output behavior explicit: add --load when the command is intended to create a
local image, or document the intended --push workflow if the image should be
published.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1677d333-72b6-42eb-95c5-b00635dbcb96
📒 Files selected for processing (10)
README.mdREADME_cn.mdascend-vnpu-monitor-integration.yamlcharts/ascend-device-plugin/README.mdcharts/ascend-device-plugin/templates/vnpu-monitor-integration.yamlcharts/ascend-device-plugin/values.yamldocs/hami.mddocs/hami_cn.mddocs/volcano.mddocs/volcano_cn.md
💤 Files with no reviewable changes (3)
- charts/ascend-device-plugin/templates/vnpu-monitor-integration.yaml
- ascend-vnpu-monitor-integration.yaml
- charts/ascend-device-plugin/values.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/hami_cn.md (1)
70-72: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClarify that
1is the resource-request quantity, notresourceName.
resourceNameis the resource key, as shown in the preceding sentence and YAML examples. The current wording may lead users to configure an invalid resource name instead of requesting one unit of the selected resource.Proposed wording
-如果多个任务要共享同一张卡，需要将 resourceName 设置为 1，并且设置对应的 ResourceMemoryName。 +如果多个任务要共享同一张卡，需要将对应 resourceName 的请求值设置为 1，并设置对应的 resourceMemoryName。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/hami_cn.md` around lines 70 - 72, 更新 docs/hami_cn.md 中关于共享同一张卡的说明，明确将资源请求数量设置为 1，而不是将 resourceName 设置为 1；保留 resourceName 作为所选资源键，并继续说明需同时配置对应的 ResourceMemoryName。
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/hami_cn.md`:
- Around line 44-48: Update the kubectl apply command in the “不同命名空间” section to
explicitly target the ascend-device-plugin namespace with the appropriate
namespace flag, ensuring the ConfigMap is deployed where the instructions
require it.
---
Outside diff comments:
In `@docs/hami_cn.md`:
- Around line 70-72: 更新 docs/hami_cn.md 中关于共享同一张卡的说明，明确将资源请求数量设置为 1，而不是将
resourceName 设置为 1；保留 resourceName 作为所选资源键，并继续说明需同时配置对应的 ResourceMemoryName。
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 02cccda1-7bef-4653-88d4-e67f9a814608
📒 Files selected for processing (6)
README.mdREADME_cn.mddocs/hami.mddocs/hami_cn.mddocs/volcano.mddocs/volcano_cn.md
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- docs/volcano.md
- docs/hami.md
Signed-off-by: limengxuan <mengxuan.li@dynamia.ai>
Signed-off-by: limengxuan <mengxuan.li@dynamia.ai>
Signed-off-by: limengxuan <mengxuan.li@dynamia.ai>
Signed-off-by: limengxuan <mengxuan.li@dynamia.ai>
Signed-off-by: limengxuan <mengxuan.li@dynamia.ai>
| 2. `/usr/local/sbin/npu-smi` | ||
| 3. `/usr/local/bin/npu-smi` | ||
|
|
||
| If your host ships `npu-smi` elsewhere, add a hostPath mount for it in `ascend-device-plugin.yaml`. |
There was a problem hiding this comment.
Only /usr/local/sbin is mounted by default.
ascend-device-plugin/ascend-device-plugin.yaml
Lines 120 to 123 in 11fdee2
There was a problem hiding this comment.
Moreover, if npu-smi is located elsewhere, applyDeviceShare will fail.
ascend-device-plugin/internal/server/device_share.go
Lines 37 to 41 in 11fdee2
There was a problem hiding this comment.
yes, so i add the description here: "If your host ships npu-smi elsewhere, add a hostPath mount for it in ascend-device-plugin.yaml."
There was a problem hiding this comment.
it's the default behavior according to the document before i re-organize, we can see if it raises any issue, and decide how we deal with it
There was a problem hiding this comment.
ok, we can fix it in a separate PR
2 participants
/kind documentation
The original README is complicated and hard for user to get started. So, i re-organized a bit:
Since this project support two schedulers:
I divide the original README into two documents, each focused on a scheduler, to improve user-experience.
I also deleted the monitor-integration, because that's not part of our scope
Summary by CodeRabbit