feat: disable RDMA option when no Thunderbolt 5 hardware#1505
feat: disable RDMA option when no Thunderbolt 5 hardware#1505AlexCheema wants to merge 1 commit intomainfrom
Conversation
When the cluster has fewer than 2 nodes with Thunderbolt 5 hardware, the MLX RDMA button is now disabled with a tooltip explaining that RDMA requires Thunderbolt 5. This prevents users from selecting an instance type that will fail at placement time. Closes #1351 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR #1505 Review: "feat: disable RDMA option when no Thunderbolt 5 hardware"Clean, focused PR. 1 file changed, +51/-21. CI all green. Closes #1351. SummaryAdds a What's Done Well
Minor Concerns
VerdictClean and correct. The auto-fallback for stale RDMA selection (concern #1) is the only functional issue worth addressing before merge. |
Code Review: PR #1505 — feat: disable RDMA option when no Thunderbolt 5 hardwareAuthor: AlexCheema Closes #1351. OverviewAdds a Correctness — hasRdmaCapableNodesPASS. The derived value: This checks:
The existing Correctness — Button BehaviorPASS. The RDMA button changes:
Correctness — Tooltip ImplementationPASS. The button is now wrapped in a
The tooltip only renders when Edge Case — Stale SelectionNOTE. If a user previously selected MlxIbv (saved to localStorage) and then Edge Case — Data LoadingSAFE. Before tbIdentifiers loads (initial page load), NitsNone. VerdictLGTM. Clean UX improvement that prevents users from selecting an instance type |
Code Review — PR #1505: Disable RDMA option when no Thunderbolt 5 hardwareCI: ALL PASSING (aarch64-darwin, x86_64-linux, aarch64-linux) OverviewSingle file change (+51/-21) in Correctness — hasRdmaCapableNodesconst hasRdmaCapableNodes = $derived.by(() => {
const ids = tbIdentifiers;
if (!ids) return false;
const tb5NodeCount = Object.values(ids).filter(
(node) => node.interfaces.length > 0,
).length;
return tb5NodeCount >= 2;
});Checks Data loading edge case: Before Correctness — button behaviorFour layers of protection:
TooltipWrapped in Edge case — stale localStorage selectionIf a user previously selected VerdictLGTM. Clean, focused UX improvement — prevents users from selecting an instance type that would fail at placement time. TB5 detection logic is consistent with the existing |
Review: PR #1505 — feat: disable RDMA option when no Thunderbolt 5 hardwareSummaryDashboard-only change that disables the MLX RDMA button when fewer than 2 nodes have Thunderbolt 5 hardware. Shows a hover tooltip explaining the requirement. Prevents users from selecting an instance type that would fail at placement time. AnalysisDetection logic is correct. UI implementation is clean:
Edge case handled: If a user had previously selected RDMA (saved in Note: This PR uses VerdictClean, focused UX improvement that prevents a confusing failure mode. LGTM. |
Code Review: PR #1505 —
|
Code Review: PR #1505 — feat: disable RDMA option when no Thunderbolt 5 hardwareSummaryDashboard UX improvement: disables the MLX RDMA button when the cluster has fewer than 2 nodes with TB5 hardware, with a tooltip explaining the requirement. Review
const tb5NodeCount = Object.values(ids).filter(
(node) => node.interfaces.length > 0,
).length;
return tb5NodeCount >= 2;Checks that at least 2 nodes have TB5 interfaces. ✅ UX is good:
Issues1. The check tests for interfaces, not specifically TB5 2. Still uses VerdictGood UX improvement. The main concern is the MlxIbv vs MlxJaccl naming, which may conflict with other in-flight PRs. LGTM with the naming caveat. |
Code Review -- PR #1505: feat: disable RDMA option when no Thunderbolt 5 hardwareCI: ALL PASSING (aarch64-darwin, x86_64-linux, aarch64-linux) OverviewSingle-file change (+51/-21) in Critical: Second RDMA button is not guardedThere are two RDMA instance type selectors in this file. The main one (~line 2892) is updated by this PR, but the compact/sidebar version at lines 4659-4682 (labeled "RDMA (Fast)") is completely unguarded: <!-- lines 4659-4682: NOT updated by this PR -->
<button
onclick={() => {
selectedInstanceType = "MlxIbv";
saveLaunchDefaults();
}}
class="... cursor-pointer {selectedInstanceType === 'MlxIbv' ...}"
>
...
RDMA (Fast)
</button>A user can select RDMA through this second button even when Significant: Stale
|
Summary
hasRdmaCapableNodesderived check that detects if >=2 nodes have Thunderbolt 5 hardwarecursor-not-allowed)Closes #1351
Test plan
npm run buildsucceedsuv run basedpyright— 0 errorsnix fmt— 0 files changed🤖 Generated with Claude Code