Skip to content

Reduce data that is transmitted and stored for memory devices - #4733

Draft
terickson-nvidia wants to merge 3 commits into
NVIDIA:mainfrom
terickson-nvidia:tom/memory-devices
Draft

Reduce data that is transmitted and stored for memory devices#4733
terickson-nvidia wants to merge 3 commits into
NVIDIA:mainfrom
terickson-nvidia:tom/memory-devices

Conversation

@terickson-nvidia

@terickson-nvidia terickson-nvidia commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

For better scaling, this PR reduces the size of data transmitted and stored for memory devices. It adds a count field to {size, type} so that repeating MemoryDevice records can be condensed into a single MemoryDeviceGroup record. Wherever the code expects the MemoryDevice list, the condensed list is rehydrated from the new internal format.

Related issues

see also #2936

Type of Change

  • Add - New feature or capability
  • Change - Changes in existing functionality
  • Fix - Bug fixes
  • Remove - Removed features or deprecated functionality
  • Internal - Internal changes (refactoring, tests, docs, etc.)

Breaking Changes

  • This PR contains breaking changes

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed
  • No testing required (docs, internal refactor, etc.)

Additional Notes

@terickson-nvidia
terickson-nvidia requested a review from a team as a code owner August 7, 2026 22:35
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Summary by CodeRabbit

  • New Features

    • Memory inventory now groups identical memory devices and records their quantity.
    • Memory totals, capacity breakdowns, and component counts accurately include device quantities.
    • Hardware discovery and host displays support grouped memory details.
  • Bug Fixes

    • Corrected calculations that previously counted each device entry only once.
    • Added compatibility for reading legacy, ungrouped memory data.
  • Refactor

    • Deprecated flat memory reporting for new data while preserving legacy read support.

Walkthrough

Memory inventory now uses grouped records with device counts. Protocols, discovery, RPC conversion, display formatting, capability totals, SKU generation, fixtures, and legacy compatibility handling were updated.

Changes

Grouped memory device flow

Layer / File(s) Summary
Memory grouping contracts
crates/api-model/src/hardware_info.rs, crates/rpc/proto/..., rest-api/proto/...
Added MemoryDeviceGroup, count-based expansion and condensation, and legacy deserialization support. Deprecated flat RPC fields remain readable.
RPC conversion and compatibility
crates/rpc/src/model/hardware_info.rs
RPC conversion prefers grouped data, condenses legacy flat data, emits grouped data, and expands groups for flat projections.
Discovery and memory display
crates/host-support/..., crates/machine-a-tron/..., crates/api-web/..., crates/rpc-utils/...
Discovery producers write grouped records. Display formatting uses group counts and falls back to legacy flat records.
Count-aware aggregation and fixtures
crates/api-model/src/machine/capabilities.rs, crates/api-db/src/sku.rs, crates/api-model/src/test_support/...
Capability totals and SKU aggregation multiply memory size by count. Hardware fixtures use grouped records.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant HardwareDiscovery
  participant DiscoveryInfo
  participant RpcHardwareInfo
  participant ManagedHostDisplay
  HardwareDiscovery->>DiscoveryInfo: write memory_device_groups
  DiscoveryInfo->>RpcHardwareInfo: convert grouped discovery data
  RpcHardwareInfo->>ManagedHostDisplay: provide grouped memory data
  ManagedHostDisplay->>ManagedHostDisplay: multiply sizes and counts
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 59.46% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description clearly explains the memory-device grouping change, its scaling goal, compatibility approach, and testing status.
Title check ✅ Passed The title clearly summarizes the primary change: reducing transmitted and stored memory-device data.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

🔐 TruffleHog Secret Scan

No secrets or credentials found!

Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉

🔗 View scan details

🕐 Last updated: 2026-08-08 00:01:25 UTC | Commit: 28c67ec

@coderabbitai coderabbitai 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.

Actionable comments posted: 1

🤖 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 `@crates/rpc/proto/machine_discovery.proto`:
- Around line 206-210: Define and apply one explicit zero-count contract for
MemoryDeviceGroup across all grouped-memory paths. In
crates/rpc/proto/machine_discovery.proto lines 206-210 and
rest-api/proto/core/src/v1/machine_discovery_nico.proto lines 211-215, document
whether zero is invalid or normalizes to one; in
crates/api-model/src/hardware_info.rs lines 290-315 and
crates/rpc-utils/src/managed_host_display.rs lines 544-557, enforce that
contract before merging or calculating totals. In
crates/rpc/src/model/hardware_info.rs lines 2134-2184, add a table-driven
compatibility case covering zero counts and the chosen normalized or rejected
result.
🪄 Autofix

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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: bce7494d-d4b8-401d-aa89-79f2b15d1523

📥 Commits

Reviewing files that changed from the base of the PR and between e05a814 and b9cec8b.

⛔ Files ignored due to path filters (1)
  • rest-api/proto/core/gen/v1/machine_discovery_nico.pb.go is excluded by !**/*.pb.go, !**/gen/**, !rest-api/**/*.pb.go
📒 Files selected for processing (11)
  • crates/api-db/src/sku.rs
  • crates/api-model/src/hardware_info.rs
  • crates/api-model/src/machine/capabilities.rs
  • crates/api-model/src/test_support/machine_snapshot.rs
  • crates/api-web/src/managed_host.rs
  • crates/host-support/src/hardware_enumeration.rs
  • crates/machine-a-tron/src/discovery_info.rs
  • crates/rpc-utils/src/managed_host_display.rs
  • crates/rpc/proto/machine_discovery.proto
  • crates/rpc/src/model/hardware_info.rs
  • rest-api/proto/core/src/v1/machine_discovery_nico.proto

Comment thread crates/rpc/proto/machine_discovery.proto
@terickson-nvidia
terickson-nvidia marked this pull request as draft August 7, 2026 22:59
@copy-pr-bot

copy-pr-bot Bot commented Aug 7, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@copy-pr-bot

copy-pr-bot Bot commented Aug 7, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@terickson-nvidia

Copy link
Copy Markdown
Contributor Author

Temporarily taking out of draft to troubleshoot trufflehog false positive. Don't merge merge until tested on DPU.

@terickson-nvidia
terickson-nvidia marked this pull request as ready for review August 7, 2026 23:58
@terickson-nvidia

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Already reviewed.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@terickson-nvidia

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Full review finished.

@coderabbitai coderabbitai 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.

Actionable comments posted: 5

🤖 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 `@crates/api-db/src/sku.rs`:
- Around line 554-562: The SKU aggregation paths use unchecked arithmetic for
memory totals and component counts. In crates/api-db/src/sku.rs lines 554-562,
update generate_base_sku_from_hardware so total_mem and entry.count use widened
operands with saturating_add, or reject overflow before constructing SKU data;
apply the same policy in generate_sku_from_machine_at_version_0_or_1 at lines
414-422.

In `@crates/api-model/src/hardware_info.rs`:
- Around line 54-55: Preserve legacy compatibility across
crates/api-model/src/hardware_info.rs:54-55,
crates/rpc/src/model/hardware_info.rs:564-570,
crates/host-support/src/hardware_enumeration.rs:916-918, and
crates/machine-a-tron/src/discovery_info.rs:232-234, 289-291, 356-358, 447-449,
and 580-582 by populating both memory_devices and memory_device_groups in
outgoing DiscoveryInfo. Keep the grouped-to-flat fallback for reads, and use
capability gating or a documented migration/rollout plan so legacy clients
receive correctly expanded devices rather than one device per group when count
exceeds one.

In `@crates/api-model/src/machine/capabilities.rs`:
- Around line 352-360: Use a consistent zero-count policy for grouped-memory
aggregation: skip any MemoryDeviceGroup with count == 0 before updating results.
Apply this in crates/api-model/src/machine/capabilities.rs:352-360 around the
mem_map insertion, crates/api-db/src/sku.rs:414-422 before SKU totals/components
updates, and crates/api-db/src/sku.rs:554-562 within
generate_base_sku_from_hardware; define and add tests covering the zero-count
behavior.
- Around line 352-360: Update the memory capability aggregation around
MemoryDeviceGroup::rehydrate so groups with count == 0 do not insert or
contribute a zero-valued capability; skip them before calculating or updating
mem_map. Define the zero-count behavior explicitly and add or update tests
covering that no capability entry is produced for such groups.

In `@crates/machine-a-tron/src/discovery_info.rs`:
- Line 979: Update the test around the discovery memory group assertion to
compare the complete expected MemoryDeviceGroup, including count, size, memory
type, and the 983,040 MiB inventory, rather than checking only len(). Also
update the related tests to preserve the count and verify discovery round trips.
🪄 Autofix

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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: d5cd1f6f-28fb-45cd-910f-b9b8a6f8b90e

📥 Commits

Reviewing files that changed from the base of the PR and between 70a9804 and 088894c.

⛔ Files ignored due to path filters (1)
  • rest-api/proto/core/gen/v1/machine_discovery_nico.pb.go is excluded by !**/*.pb.go, !**/gen/**, !rest-api/**/*.pb.go
📒 Files selected for processing (11)
  • crates/api-db/src/sku.rs
  • crates/api-model/src/hardware_info.rs
  • crates/api-model/src/machine/capabilities.rs
  • crates/api-model/src/test_support/machine_snapshot.rs
  • crates/api-web/src/managed_host.rs
  • crates/host-support/src/hardware_enumeration.rs
  • crates/machine-a-tron/src/discovery_info.rs
  • crates/rpc-utils/src/managed_host_display.rs
  • crates/rpc/proto/machine_discovery.proto
  • crates/rpc/src/model/hardware_info.rs
  • rest-api/proto/core/src/v1/machine_discovery_nico.proto

Comment thread crates/api-db/src/sku.rs
Comment on lines +554 to +562
total_mem += cap as u64 * mem.count as u64;
let key = (mem.mem_type.clone().unwrap_or_default(), cap);
mem_components
.entry(key.clone())
.and_modify(|entry| entry.count += 1)
.and_modify(|entry| entry.count += mem.count)
.or_insert(SkuComponentMemory {
capacity_mb: key.1,
memory_type: key.0,
count: 1,
count: mem.count,

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.

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Use bounded arithmetic in both SKU aggregation paths.

The new count-aware aggregation uses unchecked additions. Use widened operands with saturating_add, or validate and reject overflowing counts before constructing SKU data.

  • crates/api-db/src/sku.rs#L554-L562: make total_mem and entry.count overflow-safe in generate_base_sku_from_hardware.
  • crates/api-db/src/sku.rs#L414-L422: apply the same arithmetic policy in generate_sku_from_machine_at_version_0_or_1.
📍 Affects 1 file
  • crates/api-db/src/sku.rs#L554-L562 (this comment)
  • crates/api-db/src/sku.rs#L414-L422
🤖 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 `@crates/api-db/src/sku.rs` around lines 554 - 562, The SKU aggregation paths
use unchecked arithmetic for memory totals and component counts. In
crates/api-db/src/sku.rs lines 554-562, update generate_base_sku_from_hardware
so total_mem and entry.count use widened operands with saturating_add, or reject
overflow before constructing SKU data; apply the same policy in
generate_sku_from_machine_at_version_0_or_1 at lines 414-422.

Comment on lines +54 to +55
#[serde(default, deserialize_with = "deserialize_and_condense_memory_devices")]
pub memory_devices: Vec<MemoryDeviceGroup>,

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.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Map the conversion and model surfaces before inspecting consumers.
ast-grep outline crates/rpc/src/model/hardware_info.rs --items all --type impl,function
ast-grep outline crates/api-model/src/hardware_info.rs --items all --type struct,function

# Find consumers that still read only the deprecated flat representation.
rg -n --type rust -C 4 '\.memory_devices\b|memory_devices:' crates rest-api

# Find grouped-memory readers and writers that require a coordinated rollout.
rg -n --type rust -C 4 '\.memory_device_groups\b|memory_device_groups:' crates rest-api

Repository: NVIDIA/infra-controller

Length of output: 203


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- API model ---'
sed -n '1,130p' crates/api-model/src/hardware_info.rs

printf '%s\n' '--- RPC model conversion ---'
sed -n '500,610p' crates/rpc/src/model/hardware_info.rs

printf '%s\n' '--- Host enumeration ---'
sed -n '850,950p' crates/host-support/src/hardware_enumeration.rs

printf '%s\n' '--- All flat-memory references ---'
rg -n --type rust -C 3 '\bmemory_devices\b' crates rest-api || true

printf '%s\n' '--- All grouped-memory references ---'
rg -n --type rust -C 3 '\bmemory_device_groups\b' crates rest-api || true

Repository: NVIDIA/infra-controller

Length of output: 40329


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- JSON memory types and deserializer ---'
sed -n '230,315p' crates/api-model/src/hardware_info.rs
rg -n -C 5 'message (DiscoveryInfo|MemoryDevice|MemoryDeviceGroup)|memory_device_groups|memory_devices' . -g '*.proto' -g '*.rs' -g '*.toml' | head -n 240

printf '%s\n' '--- Current change summary ---'
git diff --stat HEAD^
git diff -- crates/api-model/src/hardware_info.rs crates/rpc/src/model/hardware_info.rs crates/host-support/src/hardware_enumeration.rs crates/machine-a-tron/src/discovery_info.rs

printf '%s\n' '--- Repository references to compatibility and rollout ---'
rg -n -i -C 3 'memory.*(compat|legacy|deprecated|migration)|compat.*memory|non.break|mixed.version|rollback' crates rest-api README.md STYLE_GUIDE.md 2>/dev/null || true

Repository: NVIDIA/infra-controller

Length of output: 50380


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- Proto files ---'
fd -e proto . | head -n 80

printf '%s\n' '--- Memory schema declarations ---'
rg -n --glob '*.proto' -C 8 'memory_device_groups|memory_devices|message MemoryDevice' . || true

printf '%s\n' '--- Deserializer and memory model ---'
sed -n '250,310p' crates/api-model/src/hardware_info.rs
rg -n -C 6 'struct MemoryDevice(Group)?|impl.*MemoryDevice(Group)?|fn rehydrate|deserialize_and_condense' crates/api-model/src/hardware_info.rs

printf '%s\n' '--- Focused parent diff ---'
git diff --unified=25 HEAD^ -- crates/api-model/src/hardware_info.rs crates/rpc/src/model/hardware_info.rs crates/host-support/src/hardware_enumeration.rs crates/machine-a-tron/src/discovery_info.rs | sed -n '1,1000p'

Repository: NVIDIA/infra-controller

Length of output: 24800


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- Previous JSON model ---'
git show HEAD^:crates/api-model/src/hardware_info.rs | sed -n '25,75p'
git show HEAD^:crates/api-model/src/hardware_info.rs | sed -n '205,285p'

printf '%s\n' '--- Current serialization and JSONB boundaries ---'
rg -n -C 5 'HardwareInfo|hardware_info|serde_json|Jsonb|JSONB' crates/api-* crates/rpc/src/model/hardware_info.rs \
  -g '*.rs' | grep -E 'hardware_info|HardwareInfo|serde_json|Jsonb|JSONB' | head -n 240

printf '%s\n' '--- Compatibility documentation around the new field ---'
sed -n '25,65p' crates/rpc/proto/machine_discovery.proto
sed -n '35,75p' rest-api/proto/core/src/v1/machine_discovery_nico.proto

printf '%s\n' '--- Legacy and grouped RPC conversion tests ---'
sed -n '2025,2205p' crates/rpc/src/model/hardware_info.rs

Repository: NVIDIA/infra-controller

Length of output: 25482


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- Memory migration history ---'
git log --oneline -20 -- crates/api-model/src/hardware_info.rs crates/rpc/proto/machine_discovery.proto
git log --all --oneline -S'memory_device_groups' -- crates/api-model/src/hardware_info.rs crates/rpc/proto/machine_discovery.proto | head -n 20

printf '%s\n' '--- Rollout and version support references ---'
rg -n -i --glob '*.md' --glob '*.rs' --glob '*.yaml' --glob '*.yml' \
  'mixed.version|backward.compat|backwards.compat|rollback|wire.compat|deprecated.*memory|memory_device_groups|non.breaking' \
  crates rest-api README.md STYLE_GUIDE.md 2>/dev/null | head -n 240 || true

printf '%s\n' '--- Exact JSON shape produced by current model ---'
sed -n '220,305p' crates/api-model/src/hardware_info.rs

Repository: NVIDIA/infra-controller

Length of output: 26523


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- Condensing change and affected writers ---'
git show --stat --oneline b9cec8b0
git show --format= --unified=12 b9cec8b0 -- \
  crates/api-model/src/hardware_info.rs \
  crates/rpc/src/model/hardware_info.rs \
  crates/host-support/src/hardware_enumeration.rs \
  crates/machine-a-tron/src/discovery_info.rs \
  crates/rpc/proto/machine_discovery.proto | sed -n '1,1200p'

Repository: NVIDIA/infra-controller

Length of output: 40246


Preserve compatibility for legacy memory readers.

When mixed-version deployments or rollback remain supported, populate memory_devices as well as memory_device_groups in outgoing DiscoveryInfo. Current writers send an empty legacy field, so legacy RPC clients receive no memory devices.

The JSONB representation changes flat entries into grouped objects with count. A legacy JSON reader treats each group as one device and undercounts memory when count > 1.

Use a capability-gated projection or a documented migration and rollout plan before treating this change as non-breaking. Retain the grouped-to-flat fallback for reads.

📍 Affects 4 files
  • crates/api-model/src/hardware_info.rs#L54-L55 (this comment)
  • crates/rpc/src/model/hardware_info.rs#L564-L570
  • crates/host-support/src/hardware_enumeration.rs#L916-L918
  • crates/machine-a-tron/src/discovery_info.rs#L232-L234
  • crates/machine-a-tron/src/discovery_info.rs#L289-L291
  • crates/machine-a-tron/src/discovery_info.rs#L356-L358
  • crates/machine-a-tron/src/discovery_info.rs#L447-L449
  • crates/machine-a-tron/src/discovery_info.rs#L580-L582
🤖 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 `@crates/api-model/src/hardware_info.rs` around lines 54 - 55, Preserve legacy
compatibility across crates/api-model/src/hardware_info.rs:54-55,
crates/rpc/src/model/hardware_info.rs:564-570,
crates/host-support/src/hardware_enumeration.rs:916-918, and
crates/machine-a-tron/src/discovery_info.rs:232-234, 289-291, 356-358, 447-449,
and 580-582 by populating both memory_devices and memory_device_groups in
outgoing DiscoveryInfo. Keep the grouped-to-flat fallback for reads, and use
capability gating or a documented migration/rollout plan so legacy clients
receive correctly expanded devices rather than one device per group when count
exceeds one.

Source: Path instructions

Comment on lines +352 to +360
let total = mem_info
.size_mb
.unwrap_or_default()
.saturating_mul(mem_info.count) as usize;

mem_map
.entry(name)
.and_modify(|e| {
*e = e.saturating_add(mem_info.size_mb.unwrap_or_default() as usize)
})
.or_insert_with(|| mem_info.size_mb.unwrap_or_default() as usize);
.and_modify(|e| *e = e.saturating_add(total))
.or_insert(total);

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.

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Use one explicit zero-count policy across memory aggregations.

MemoryDeviceGroup::rehydrate() treats count == 0 as no devices, but the aggregation paths emit zero-valued records. Skip zero-count groups before aggregation, or reject them at the model boundary.

  • crates/api-model/src/machine/capabilities.rs#L352-L360: skip zero-count groups before inserting into mem_map.
  • crates/api-db/src/sku.rs#L414-L422: skip zero-count groups before updating SKU totals and components.
  • crates/api-db/src/sku.rs#L554-L562: apply the same skip in generate_base_sku_from_hardware.

As per path instructions, grouped-memory changes must define and test “zero-count behavior”.

📍 Affects 2 files
  • crates/api-model/src/machine/capabilities.rs#L352-L360 (this comment)
  • crates/api-db/src/sku.rs#L414-L422
  • crates/api-db/src/sku.rs#L554-L562
🤖 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 `@crates/api-model/src/machine/capabilities.rs` around lines 352 - 360, Use a
consistent zero-count policy for grouped-memory aggregation: skip any
MemoryDeviceGroup with count == 0 before updating results. Apply this in
crates/api-model/src/machine/capabilities.rs:352-360 around the mem_map
insertion, crates/api-db/src/sku.rs:414-422 before SKU totals/components
updates, and crates/api-db/src/sku.rs:554-562 within
generate_base_sku_from_hardware; define and add tests covering the zero-count
behavior.

Source: Path instructions


🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Skip zero-count memory groups.

MemoryDeviceGroup::rehydrate() in crates/api-model/src/hardware_info.rs Lines 256-267 produces no devices when count == 0. This loop still inserts a 0 MB capability. Skip zero-count groups or reject them at the model boundary.

Proposed fix
 for mem_info in hardware_info.memory_devices.iter() {
+    if mem_info.count == 0 {
+        continue;
+    }
     let name = mem_info

As per path instructions, grouped-memory changes must define and test “zero-count behavior”.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let total = mem_info
.size_mb
.unwrap_or_default()
.saturating_mul(mem_info.count) as usize;
mem_map
.entry(name)
.and_modify(|e| {
*e = e.saturating_add(mem_info.size_mb.unwrap_or_default() as usize)
})
.or_insert_with(|| mem_info.size_mb.unwrap_or_default() as usize);
.and_modify(|e| *e = e.saturating_add(total))
.or_insert(total);
if mem_info.count == 0 {
continue;
}
let total = mem_info
.size_mb
.unwrap_or_default()
.saturating_mul(mem_info.count) as usize;
mem_map
.entry(name)
.and_modify(|e| *e = e.saturating_add(total))
.or_insert(total);
🤖 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 `@crates/api-model/src/machine/capabilities.rs` around lines 352 - 360, Update
the memory capability aggregation around MemoryDeviceGroup::rehydrate so groups
with count == 0 do not insert or contribute a zero-valued capability; skip them
before calculating or updating mem_map. Define the zero-count behavior
explicitly and add or update tests covering that no capability entry is produced
for such groups.

Source: Path instructions

[2, 1, 4, 3]
);
assert_eq!(discovery.memory_devices.len(), 2);
assert_eq!(discovery.memory_device_groups.len(), 1);

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.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Assert the complete memory group.

Line 979 only checks that one group exists. A wrong count, size, or memory type still passes. Assert the expected MemoryDeviceGroup value so this profile preserves its 983,040 MiB memory inventory.

As per path instructions, update tests for count preservation and round trips.

🤖 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 `@crates/machine-a-tron/src/discovery_info.rs` at line 979, Update the test
around the discovery memory group assertion to compare the complete expected
MemoryDeviceGroup, including count, size, memory type, and the 983,040 MiB
inventory, rather than checking only len(). Also update the related tests to
preserve the count and verify discovery round trips.

Source: Path instructions

@terickson-nvidia
terickson-nvidia marked this pull request as draft August 8, 2026 01:27
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.

1 participant