Skip to content

build: prototype symlinked dual SDK namespaces#1327

Draft
jasonqinzhou wants to merge 4 commits into
mainfrom
agent/symlink-core-sdk-namespaces
Draft

build: prototype symlinked dual SDK namespaces#1327
jasonqinzhou wants to merge 4 commits into
mainfrom
agent/symlink-core-sdk-namespaces

Conversation

@jasonqinzhou

@jasonqinzhou jasonqinzhou commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

This is an alternative packaging prototype to #1322. It keeps the SDK, system data, model data, collector, and Rust crate in the repository's existing layout instead of relocating the project into packages/.

The SDK has one real source tree:

src/aiconfigurator/sdk/
src/aiconfigurator_core/sdk -> ../aiconfigurator/sdk

The relative Git symlink exposes the full SDK through both public import paths:

from aiconfigurator.sdk.task_v2 import Task
from aiconfigurator_core.sdk.task_v2 import Task

Packaging behavior

  • aiconfigurator-core is a standalone Maturin wheel containing the Rust extension, SDK, system/model data, and both SDK namespace trees.
  • Maturin follows the source symlink and materializes its target as ordinary files in the wheel. Installed wheels do not contain filesystem symlinks.
  • The upper aiconfigurator wheel contains CLI, generator, webapp, and Spica, and depends exactly on aiconfigurator-core==0.10.0.
  • The two distributions have disjoint payload ownership.
  • Core-only installation supports every SDK leaf through both namespaces, including Task and EngineHandle.

Important tradeoffs

This is source deduplication, not Python module aliasing. Importing the same leaf through both names creates separate module objects, classes, registries, and caches. Applications must choose one namespace consistently and should not exchange objects imported from the two namespaces. This is the principal semantic difference from #1322's compatibility wrappers, which preserve exact module identity.

There are two additional packaging caveats:

  • A normal dependency-first pip upgrade from the monolithic 0.9 wheel can remove freshly installed core-owned aiconfigurator/sdk files while uninstalling the old package. The tested migration paths are uninstall-first or force-reinstalling core after the upgrade.
  • Windows checkouts must have Git symlink support enabled. The release wheel itself is portable with respect to the link because the wheel entries are ordinary files.

These limitations are intentionally documented in the draft so the symlink design can be compared directly with #1322 before choosing the release structure.

Validation

  • Full unit suite: 1792 passed, 36 skipped.
  • Targeted namespace/layering/memory tests: 63 passed.
  • Every one of the 59 non-package SDK leaves imports through both namespaces in isolated subprocesses.
  • Release audit: 175 + 847 disjoint upper/core payload files.
  • Both SDK namespace trees contain 61 byte-identical regular Python files.
  • Core-only, full, upper-uninstall, core-uninstall, and reinstall lifecycle checks passed.
  • Native core smoke test and real EngineHandle latency predictions passed.
  • uv lock --check, Ruff, formatting, and git diff --check passed.

Summary by CodeRabbit

  • New Features
    • Added support for installing and verifying the app as separate “upper” and “core” packages.
    • Enhanced wheel/distribution validation for correct SDK namespace and payload ownership.
  • Bug Fixes
    • Improved systems-path resolution and forwarding for embedded and runtime estimates.
    • Improved handling for model/config locations via environment override.
  • Documentation
    • Updated README/DEVELOPMENT/tests with the new install order, upgrade steps, and SDK namespace/symlink behavior.
    • Clarified platform support for published core wheels.

jasonqinzhou and others added 3 commits July 7, 2026 19:04
Signed-off-by: jasonqinzhou <jasonqinzhou@users.noreply.github.com>
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Jul 8, 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.

@github-actions github-actions Bot added the build label Jul 8, 2026
@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 563919f7-cd3a-438d-86cb-b9f66ed663d1

📥 Commits

Reviewing files that changed from the base of the PR and between cbd64cc and b04ad5c.

📒 Files selected for processing (2)
  • src/aiconfigurator/sdk/logging_utils.py
  • tests/unit/sdk/test_core_namespace.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/aiconfigurator/sdk/logging_utils.py
📜 Recent review details
⏰ Context from checks skipped due to timeout. (5)
  • GitHub Check: AIC Accuracy Regression Testing
  • GitHub Check: Rust/Python engine-step parity
  • GitHub Check: Build and Test (e2e)
  • GitHub Check: Build and Test (unit)
  • GitHub Check: Cargo Deny
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py

📄 CodeRabbit inference engine (.claude/rules/generator/guard_rails.md)

**/*.py: Catch unsupported system/backend/version combinations early with fast validation to prevent wasting user time on late failures after profiling.
MoE model detection must use config.json, not model name patterns, to correctly identify MoE models with non-standard naming.
Use model_family not model_name for model compatibility checks; model_name includes size/quantization info that changes across variants.
safe_model_name must be generated BEFORE saving results to avoid race condition with raw model_path containing slashes.
Quantization type must be inferred from model config, not name patterns, to handle inconsistent naming conventions.

Files:

  • tests/unit/sdk/test_core_namespace.py
**/*

⚙️ CodeRabbit configuration file

**/*: - Prefer applicable inline comments. When the correct fix is clear, small, and limited to the commented diff hunk, include it as a GitHub Suggested Change so the author can apply it with one click.

  • Do not use a suggested change when the fix requires broader design choices, multiple files, generated artifacts, unavailable context, or validation that cannot be inferred from the diff.
  • If a comment is not directly applicable, state the smallest concrete next step and why a one-click suggestion is not safe.

Files:

  • tests/unit/sdk/test_core_namespace.py
tests/**

⚙️ CodeRabbit configuration file

tests/**: - Check that tests cover the changed behavior rather than only the happy path.

  • Watch for fixtures or golden outputs that mask backend drift, support-matrix ordering changes, or CLI output regressions.

Files:

  • tests/unit/sdk/test_core_namespace.py
🔇 Additional comments (1)
tests/unit/sdk/test_core_namespace.py (1)

10-10: LGTM!

Also applies to: 91-121


Walkthrough

This PR splits the package into upper and core distributions, updates path resolution and logging imports, adds verification tooling, and refreshes build, CI, docs, and tests for the new layout.

Changes

Package split into upper and core

Layer / File(s) Summary
Core SDK symlink and logging shim
src/aiconfigurator_core/sdk, src/aiconfigurator/logging_utils.py, src/aiconfigurator/sdk/logging_utils.py, src/aiconfigurator/sdk/pareto_analysis.py
aiconfigurator_core/sdk becomes a symlink to aiconfigurator/sdk; logging helpers move into the SDK and are re-exported from the upper package, and pareto_analysis imports the new SDK path.
Configurable systems/model-config path resolution
src/aiconfigurator/sdk/engine.py, src/aiconfigurator/sdk/memory.py, src/aiconfigurator/sdk/utils.py, rust/aiconfigurator-core/src/py.rs, rust/aiconfigurator-core/tests/*, tests/unit/sdk/test_memory_estimation.py
Systems path lookup now forwards systems_paths, model-config paths honor an env override, Rust resolves systems roots from args/env/installed core/fallback, and the embedded tests switch to installed-core resolution.
Package build configuration for core and upper wheels
pyproject.toml, src/aiconfigurator-core/pyproject.toml
The upper package now depends on aiconfigurator-core and uses setuptools, while the core package becomes a standalone maturin-built distribution with expanded runtime requirements and include rules.
Wheel/layer verification and legacy migration tooling
tools/verify_release_wheels.py, tools/verify_installed_package_layers.py, tools/build_legacy_monolith_fixture.py
Release-wheel verification now checks payload ownership boundaries and SDK mirror integrity, installed-layer checks validate each distribution, and a deterministic legacy wheel fixture is added for migration tests.
Docker build and CI workflow updates
docker/Dockerfile, .github/workflows/*
Docker and workflow steps now install the core package separately, build and verify both wheels, and run container/parity checks with the updated import and build contexts.
Documentation updates and namespace/layering tests
DEVELOPMENT.md, README.md, tests/README.md, tests/unit/sdk/test_core_namespace.py, tests/unit/sdk/test_package_layering.py
Docs describe the split install flow and symlink layout, and tests cover namespace import behavior, logging state isolation, and SDK layering rules.

Estimated code review effort: 4 (Complex) | ~75 minutes

Poem

Two wheels now spin in matched new lanes,
A symlink hums through shared-code veins.
Paths resolve, the logs stay bright,
Tests guard the boundary day and night.
Old monoliths give way with grace —
split burrows, same warm rabbit place.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately captures the symlinked dual-namespace SDK packaging prototype.
Description check ✅ Passed The description is detailed and covers the change, tradeoffs, and validation, though it doesn't match the template headings exactly.
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.

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

@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: 2

🧹 Nitpick comments (4)
rust/aiconfigurator-core/src/py.rs (1)

82-98: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Consider distinguishing "not installed" from "installed but errored" fallback semantics.

Only the py.import(...) failure short-circuits to Ok(None); a successful import followed by a call_method0/extract error propagates via ? out of the closure, which compile_engine_from_flat converts to AicError::DataRoot — bypassing the final repo-relative fallback entirely even though it's the same style of "resolution failed" scenario the function otherwise tries to gracefully degrade through. This is a design choice, not necessarily wrong (a real error from an installed aiconfigurator-core may deserve surfacing), but worth confirming it's intentional given the rest of the function's degrade-through-multiple-sources philosophy.

🤖 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 `@rust/aiconfigurator-core/src/py.rs` around lines 82 - 98, The path resolution
in py.rs treats only import failure as “not installed,” but lets
get_systems_paths()/extract() errors escape and skip the repo-relative fallback.
Update the resolve logic around Python::with_gil and the perf_database lookup so
that “installed but lookup failed” is handled according to the intended fallback
semantics: either map those errors to Ok(None) and continue to
crate::repo_relative(...), or explicitly separate and document them if they
should remain hard failures. Make the behavior consistent with
compile_engine_from_flat and the existing installed_root fallback flow.
.github/workflows/create-charts.yml (1)

43-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Editable core install now compiles a Rust extension — no toolchain/cache step present.

aiconfigurator-core switched to the maturin backend, so pip install -e ./src/aiconfigurator-core performs a Rust build. ubuntu-latest ships a Rust toolchain, so this likely works, but there's no dtolnay/rust-toolchain pin or Swatinem/rust-cache, meaning builds are unpinned and recompiled every run. Confirm the build succeeds here and consider adding Rust caching to keep this chart-only workflow fast.

🤖 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 @.github/workflows/create-charts.yml around lines 43 - 44, The editable
install of aiconfigurator-core now triggers a Rust build, so the create-charts
workflow should either add the missing Rust toolchain/cache setup or explicitly
confirm the build path is intentional. Update the steps around the pip install
commands to include a pinned dtolnay/rust-toolchain and Swatinem/rust-cache for
the aiconfigurator-core build, or otherwise ensure the maturin-backed install is
handled efficiently and consistently in this job.

Source: Path instructions

docker/Dockerfile (1)

72-81: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Enforce wheelhouse-only resolution with --no-index.

The comment states dependency resolution "must pull the exact matching core wheel from the wheelhouse," but without --no-index uv will also consult the default index for aiconfigurator-core==<version>. Currently harmless since that exact version isn't published anywhere else yet, but it doesn't actually prove wheelhouse-exclusivity and could silently mask a resolution issue if aiconfigurator-core is ever published externally.

🔧 Proposed fix
     uv pip install --python /tmp/aic-upper-install-test/bin/python \
-        --find-links /workspace/dist "${AIC_WHL}" && \
+        --no-index --find-links /workspace/dist "${AIC_WHL}" && \
🤖 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 `@docker/Dockerfile` around lines 72 - 81, The wheel install check in the
Dockerfile still allows uv to consult the default index, so it does not באמת
enforce wheelhouse-only dependency resolution. Update the `RUN uv venv...` block
that installs `aiconfigurator-*` to add `--no-index` to the `uv pip install`
command alongside `--find-links /workspace/dist`, so the
`verify_installed_package_layers.py` check proves `aiconfigurator-core` is
resolved only from the wheelhouse.
.github/workflows/accuracy-regression-testing.yml (1)

38-42: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Consider caching cargo build artifacts for the editable core install.

pip install -e ./src/aiconfigurator-core now triggers a full maturin/Rust compile on every run of this job, with no Cargo.lock-keyed cache (e.g. actions/cache over ~/.cargo/registry and the Rust target/ dir). This adds recompilation time to every accuracy-regression run.

🤖 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 @.github/workflows/accuracy-regression-testing.yml around lines 38 - 42, The
dependency install step is repeatedly recompiling the Rust-backed editable core
install because no Cargo artifact cache is used. Update the accuracy-regression
workflow around the Install dependencies step to add caching for the
maturin/Rust build used by `pip install -e ./src/aiconfigurator-core`, ideally
keyed off `Cargo.lock`, and cache the relevant Cargo directories such as the
registry and build target so the `aiconfigurator-core` editable install can
reuse compiled artifacts across runs.
🤖 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 `@pyproject.toml`:
- Line 43: The dependency pin for aiconfigurator-core is hardcoded to
aiconfigurator version 0.10.0, which can drift from the upper wheel version used
by tools/verify_release_wheels.py. Update the pyproject.toml dependency entry so
it is derived from the project version or otherwise generated at release time,
keeping it in sync with the aiconfigurator package version and the
expected_requirement check in _verify_main_wheel.

In `@src/aiconfigurator/sdk/logging_utils.py`:
- Around line 24-39: The `use_plain_cli_output()` check is too tied to a
specific `ColoredFormatter` class identity, so it can miss `--no-color` when
logging is configured through the other namespace. Update the handler scan in
`logging_utils.use_plain_cli_output` to use namespace-agnostic duck typing
against the formatter object from each handler, checking for the
`force_no_color` signal instead of `isinstance(fmt, ColoredFormatter)`. Keep the
rest of the `_stdout_env_suggests_plain()` fallback behavior unchanged.

---

Nitpick comments:
In @.github/workflows/accuracy-regression-testing.yml:
- Around line 38-42: The dependency install step is repeatedly recompiling the
Rust-backed editable core install because no Cargo artifact cache is used.
Update the accuracy-regression workflow around the Install dependencies step to
add caching for the maturin/Rust build used by `pip install -e
./src/aiconfigurator-core`, ideally keyed off `Cargo.lock`, and cache the
relevant Cargo directories such as the registry and build target so the
`aiconfigurator-core` editable install can reuse compiled artifacts across runs.

In @.github/workflows/create-charts.yml:
- Around line 43-44: The editable install of aiconfigurator-core now triggers a
Rust build, so the create-charts workflow should either add the missing Rust
toolchain/cache setup or explicitly confirm the build path is intentional.
Update the steps around the pip install commands to include a pinned
dtolnay/rust-toolchain and Swatinem/rust-cache for the aiconfigurator-core
build, or otherwise ensure the maturin-backed install is handled efficiently and
consistently in this job.

In `@docker/Dockerfile`:
- Around line 72-81: The wheel install check in the Dockerfile still allows uv
to consult the default index, so it does not באמת enforce wheelhouse-only
dependency resolution. Update the `RUN uv venv...` block that installs
`aiconfigurator-*` to add `--no-index` to the `uv pip install` command alongside
`--find-links /workspace/dist`, so the `verify_installed_package_layers.py`
check proves `aiconfigurator-core` is resolved only from the wheelhouse.

In `@rust/aiconfigurator-core/src/py.rs`:
- Around line 82-98: The path resolution in py.rs treats only import failure as
“not installed,” but lets get_systems_paths()/extract() errors escape and skip
the repo-relative fallback. Update the resolve logic around Python::with_gil and
the perf_database lookup so that “installed but lookup failed” is handled
according to the intended fallback semantics: either map those errors to
Ok(None) and continue to crate::repo_relative(...), or explicitly separate and
document them if they should remain hard failures. Make the behavior consistent
with compile_engine_from_flat and the existing installed_root fallback flow.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: daa4e45b-ec8b-4453-a707-712cf6be9e61

📥 Commits

Reviewing files that changed from the base of the PR and between 0828d6b and cbd64cc.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock and included by none
📒 Files selected for processing (25)
  • .github/workflows/accuracy-regression-testing.yml
  • .github/workflows/build-test.yml
  • .github/workflows/create-charts.yml
  • DEVELOPMENT.md
  • README.md
  • docker/Dockerfile
  • pyproject.toml
  • rust/aiconfigurator-core/src/py.rs
  • rust/aiconfigurator-core/tests/embedded_round_trip.rs
  • rust/aiconfigurator-core/tests/memory_round_trip.rs
  • src/aiconfigurator-core/pyproject.toml
  • src/aiconfigurator/logging_utils.py
  • src/aiconfigurator/sdk/engine.py
  • src/aiconfigurator/sdk/logging_utils.py
  • src/aiconfigurator/sdk/memory.py
  • src/aiconfigurator/sdk/pareto_analysis.py
  • src/aiconfigurator/sdk/utils.py
  • src/aiconfigurator_core/sdk
  • tests/README.md
  • tests/unit/sdk/test_core_namespace.py
  • tests/unit/sdk/test_memory_estimation.py
  • tests/unit/sdk/test_package_layering.py
  • tools/build_legacy_monolith_fixture.py
  • tools/verify_installed_package_layers.py
  • tools/verify_release_wheels.py
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
  • GitHub Check: Cargo Deny
  • GitHub Check: Build and Test (unit)
  • GitHub Check: Rust/Python engine-step parity
  • GitHub Check: Build and Test (e2e)
  • GitHub Check: AIC Accuracy Regression Testing
🧰 Additional context used
📓 Path-based instructions (6)
.github/workflows/**

⚙️ CodeRabbit configuration file

.github/workflows/**: - Review workflow edits for least-privilege permissions, safe trigger scopes, secret exposure, fork behavior, and release/cherry-pick side effects.

  • Verify matrix jobs, artifact uploads, and scheduled support-matrix workflows still produce debuggable evidence.

Files:

  • .github/workflows/accuracy-regression-testing.yml
  • .github/workflows/create-charts.yml
  • .github/workflows/build-test.yml
**/*

⚙️ CodeRabbit configuration file

**/*: - Prefer applicable inline comments. When the correct fix is clear, small, and limited to the commented diff hunk, include it as a GitHub Suggested Change so the author can apply it with one click.

  • Do not use a suggested change when the fix requires broader design choices, multiple files, generated artifacts, unavailable context, or validation that cannot be inferred from the diff.
  • If a comment is not directly applicable, state the smallest concrete next step and why a one-click suggestion is not safe.

Files:

  • src/aiconfigurator_core/sdk
  • tests/README.md
  • src/aiconfigurator/sdk/engine.py
  • src/aiconfigurator/sdk/pareto_analysis.py
  • src/aiconfigurator/sdk/memory.py
  • rust/aiconfigurator-core/tests/embedded_round_trip.rs
  • src/aiconfigurator/sdk/utils.py
  • tests/unit/sdk/test_core_namespace.py
  • tests/unit/sdk/test_memory_estimation.py
  • DEVELOPMENT.md
  • src/aiconfigurator/sdk/logging_utils.py
  • src/aiconfigurator-core/pyproject.toml
  • src/aiconfigurator/logging_utils.py
  • tools/build_legacy_monolith_fixture.py
  • tests/unit/sdk/test_package_layering.py
  • pyproject.toml
  • rust/aiconfigurator-core/tests/memory_round_trip.rs
  • tools/verify_installed_package_layers.py
  • README.md
  • tools/verify_release_wheels.py
  • rust/aiconfigurator-core/src/py.rs
  • docker/Dockerfile
**/*.md

📄 CodeRabbit inference engine (.claude/rules/generator/config_schema.md)

**/*.md: Add computation rules in the rules authoring system when a new parameter requires non-trivial value computation beyond simple defaults
Add template modifications in the template authoring system when a new parameter requires special handling in generated artifacts

Files:

  • tests/README.md
  • DEVELOPMENT.md
  • README.md
tests/**

⚙️ CodeRabbit configuration file

tests/**: - Check that tests cover the changed behavior rather than only the happy path.

  • Watch for fixtures or golden outputs that mask backend drift, support-matrix ordering changes, or CLI output regressions.

Files:

  • tests/README.md
  • tests/unit/sdk/test_core_namespace.py
  • tests/unit/sdk/test_memory_estimation.py
  • tests/unit/sdk/test_package_layering.py
**/*.py

📄 CodeRabbit inference engine (.claude/rules/generator/guard_rails.md)

**/*.py: Catch unsupported system/backend/version combinations early with fast validation to prevent wasting user time on late failures after profiling.
MoE model detection must use config.json, not model name patterns, to correctly identify MoE models with non-standard naming.
Use model_family not model_name for model compatibility checks; model_name includes size/quantization info that changes across variants.
safe_model_name must be generated BEFORE saving results to avoid race condition with raw model_path containing slashes.
Quantization type must be inferred from model config, not name patterns, to handle inconsistent naming conventions.

Files:

  • src/aiconfigurator/sdk/engine.py
  • src/aiconfigurator/sdk/pareto_analysis.py
  • src/aiconfigurator/sdk/memory.py
  • src/aiconfigurator/sdk/utils.py
  • tests/unit/sdk/test_core_namespace.py
  • tests/unit/sdk/test_memory_estimation.py
  • src/aiconfigurator/sdk/logging_utils.py
  • src/aiconfigurator/logging_utils.py
  • tools/build_legacy_monolith_fixture.py
  • tests/unit/sdk/test_package_layering.py
  • tools/verify_installed_package_layers.py
  • tools/verify_release_wheels.py
src/aiconfigurator/sdk/**

⚙️ CodeRabbit configuration file

src/aiconfigurator/sdk/**: - Verify SDK API changes remain compatible with generator inputs, profiler data flow, and documented examples.

  • Flag silent schema or field-name drift between SDK models and generator/module bridge code.

Files:

  • src/aiconfigurator/sdk/engine.py
  • src/aiconfigurator/sdk/pareto_analysis.py
  • src/aiconfigurator/sdk/memory.py
  • src/aiconfigurator/sdk/utils.py
  • src/aiconfigurator/sdk/logging_utils.py
🧠 Learnings (1)
📚 Learning: 2026-05-01T00:39:37.334Z
Learnt from: simone-chen
Repo: ai-dynamo/aiconfigurator PR: 956
File: src/aiconfigurator/sdk/perf_database.py:4144-4151
Timestamp: 2026-05-01T00:39:37.334Z
Learning: In src/aiconfigurator/sdk/**/*.py, preserve upstream metadata for PerformanceResult.source: since PerformanceResult defaults source to "silicon", callers should not force-set result.source (e.g., PerfDatabase._query_silicon_or_hybrid should rely on the default and keep any existing source information rather than overwriting it). Only set source explicitly when you truly intend to change it.

Applied to files:

  • src/aiconfigurator/sdk/engine.py
  • src/aiconfigurator/sdk/pareto_analysis.py
  • src/aiconfigurator/sdk/memory.py
  • src/aiconfigurator/sdk/utils.py
  • src/aiconfigurator/sdk/logging_utils.py
🪛 ast-grep (0.44.1)
tests/unit/sdk/test_core_namespace.py

[error] 67-73: Command coming from incoming request
Context: subprocess.run(
[sys.executable, "-c", script],
cwd=REPO_ROOT,
capture_output=True,
text=True,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').

(subprocess-from-request)


[info] 56-56: use jsonify instead of json.dumps for JSON output
Context: json.dumps(leaves)
Note: [CWE-116] Improper Encoding or Escaping of Output.

(use-jsonify)

tests/unit/sdk/test_memory_estimation.py

[info] 224-224: Do not hardcode temporary file or directory names
Context: "/tmp/aic-core-systems"
Note: [CWE-377] Insecure Temporary File.

(hardcoded-tmp-file)

🪛 LanguageTool
DEVELOPMENT.md

[style] ~81-~81: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...ded wheel. The wheel itself is portable with respect to the link because it contains regular co...

(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)

README.md

[grammar] ~42-~42: Ensure spelling is correct
Context: ...tor` wheel contains the CLI, generator, webapp, and Spica. It depends on the exact mat...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

🔇 Additional comments (27)
src/aiconfigurator_core/sdk (1)

1-1: LGTM!

src/aiconfigurator/sdk/logging_utils.py (1)

56-124: LGTM! Moved verbatim from the upper package; no functional changes.

src/aiconfigurator/logging_utils.py (1)

4-22: LGTM! Clean re-export shim that preserves backward compatibility without duplicating logic.

src/aiconfigurator/sdk/pareto_analysis.py (1)

18-18: LGTM! Fixes what would otherwise be an SDK→upper-layer import violation now enforced by tests/unit/sdk/test_package_layering.py.

DEVELOPMENT.md (1)

43-44: LGTM! Documentation accurately reflects the symlink layout, matches the actual symlink target (../aiconfigurator/sdk), and clearly calls out the cross-namespace identity caveat and Windows symlink requirements.

Also applies to: 60-89, 144-146

README.md (1)

32-96: LGTM! Clear, consistent documentation of the platform constraint, namespace caveats, and upgrade path; matches the symlink layout and packaging behavior described elsewhere in the PR.

Also applies to: 113-121

tests/README.md (1)

20-20: LGTM!

rust/aiconfigurator-core/src/py.rs (1)

73-99: 🩺 Stability & Availability

No new interpreter requirement here. build_aic_engine already runs under embed-python/pyo3/auto-initialize, so Python::with_gil here matches the existing contract.

			> Likely an incorrect or invalid review comment.
src/aiconfigurator/sdk/engine.py (1)

813-819: LGTM!

src/aiconfigurator/sdk/memory.py (1)

309-309: LGTM!

src/aiconfigurator/sdk/utils.py (1)

829-835: LGTM!

rust/aiconfigurator-core/tests/embedded_round_trip.rs (1)

57-57: LGTM!

Also applies to: 96-116

rust/aiconfigurator-core/tests/memory_round_trip.rs (1)

60-73: LGTM!

Also applies to: 77-116, 119-174

tests/unit/sdk/test_memory_estimation.py (1)

206-225: LGTM!

tests/unit/sdk/test_core_namespace.py (1)

24-90: LGTM!

tests/unit/sdk/test_package_layering.py (1)

14-21: LGTM!

Also applies to: 50-78, 113-135

pyproject.toml (1)

108-122: LGTM!

tools/verify_release_wheels.py (2)

42-64: 🎯 Functional Correctness | ⚡ Quick win

rglob symlink traversal is Python-version dependent — verify coverage.

src/aiconfigurator_core/sdk is a symlink into ../aiconfigurator/sdk. Path.rglob follows symlinked directories on 3.10–3.12, but 3.13 added recurse_symlinks defaulting to False, so _add_source_tree(core, ".../aiconfigurator_core", ...) will enumerate the mirrored SDK files on older interpreters and skip them on 3.13+. That silently weakens the missing_source check depending on which interpreter runs verification. Consider walking the symlink target explicitly (or asserting the mirrored set) so coverage is deterministic across versions.


129-136: 🎯 Functional Correctness

No change needed here. The release-wheel path is Linux-only, so wheels lacking Unix mode bits are out of scope for this check.

			> Likely an incorrect or invalid review comment.
tools/verify_installed_package_layers.py (1)

126-151: 🩺 Stability & Availability

No issue here. MiniMaxAI--MiniMax-M2.5_config.json is bundled, so the --exercise-engine path already uses a packaged config.

			> Likely an incorrect or invalid review comment.
src/aiconfigurator-core/pyproject.toml (2)

35-50: 🗄️ Data Integrity & Integration

No action needed: tool.maturin.include is resolved from the pyproject.toml directory, with a fallback to python-source, so these paths are not dropped.

			> Likely an incorrect or invalid review comment.

43-50: 🗄️ Data Integrity & Integration

No recursive YAML glob needed here aiconfigurator/systems/*.yaml already covers the current flat asset layout, so there’s no nested systems/<subdir>/ YAML to add.

			> Likely an incorrect or invalid review comment.
tools/build_legacy_monolith_fixture.py (1)

1-96: LGTM!

docker/Dockerfile (2)

19-23: LGTM!

Also applies to: 33-34, 45-71


83-108: 🩺 Stability & Availability

Migration test hard-asserts on undocumented pip ordering behavior.

The if verify...; then echo ERROR...; exit 1; fi step encodes an expectation about pip's internal uninstall/install ordering when crossing a package split — behavior that isn't a documented pip contract. If a future pip release changes this ordering (e.g. to better handle overlapping RECORD entries), this Docker stage will start failing the build via the inverted assertion, even though nothing in this repo changed. The throwaway venvs here (python -m venv) also don't pin/upgrade pip, so behavior can drift silently across base-image updates.

Consider pinning pip's version for this scenario (or asserting more loosely, e.g. warning instead of hard-failing on the "unexpectedly preserved" branch) to reduce blast radius if pip's behavior changes upstream.

.github/workflows/accuracy-regression-testing.yml (1)

65-67: LGTM!

.github/workflows/build-test.yml (1)

85-85: LGTM!

Also applies to: 98-102, 234-237

Comment thread pyproject.toml
requires-python = ">=3.10"

dependencies = [
"aiconfigurator-core==0.10.0",

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

Hardcoded ==0.10.0 pin will drift from the upper wheel version.

tools/verify_release_wheels.py enforces Requires-Dist: aiconfigurator-core=={upper wheel Version} (see _verify_main_wheel, expected_requirement). This literal pin only matches while aiconfigurator is exactly 0.10.0; any version bump that forgets to edit this line will fail release verification. Consider deriving the pin from the project version (e.g. a dynamic/templated dependency or a release-time check) so the two stay in lockstep.

🤖 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 `@pyproject.toml` at line 43, The dependency pin for aiconfigurator-core is
hardcoded to aiconfigurator version 0.10.0, which can drift from the upper wheel
version used by tools/verify_release_wheels.py. Update the pyproject.toml
dependency entry so it is derived from the project version or otherwise
generated at release time, keeping it in sync with the aiconfigurator package
version and the expected_requirement check in _verify_main_wheel.

Comment thread src/aiconfigurator/sdk/logging_utils.py
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

AIC Accuracy Regression Testing

Workflow result: success

Old revision: 0828d6b7e4a7880079443b1c6f9c148d85bdbf54

Summary

  • # samples added: 0
  • ttft_mape_improvement_%: 0.000000
  • tpot_mape_improvement_%: 0.000000
Comparison summary table
partition num_samples_added ttft_mape_improvement_% tpot_mape_improvement_%
all 0 0.000000 0.000000
agg 0 0.000000 0.000000
disagg 0 0.000000 0.000000
MiniMax-M2.5 0 0.000000 0.000000
Qwen3.5-397B-A17B 0 0.000000 0.000000
Kimi-K2.5 0 0.000000 0.000000
DeepSeek-V3 0 0.000000 0.000000
DeepSeek-R1-NVFP4 0 0.000000 0.000000
DeepSeek-V3.1-NVFP4 0 0.000000 0.000000
Qwen3-32B-NVFP4 0 0.000000 0.000000
DeepSeek-R1 0 0.000000 0.000000
Qwen3-32B 0 0.000000 0.000000
Qwen3-235B-A22B-FP8 0 0.000000 0.000000
Llama-3.1-8B-Instruct-FP8 0 0.000000 0.000000
b300_sxm 0 0.000000 0.000000
gb200 0 0.000000 0.000000
b200_sxm 0 0.000000 0.000000
gb300 0 0.000000 0.000000
h200_sxm 0 0.000000 0.000000
h100_sxm 0 0.000000 0.000000
vllm-agg 0 0.000000 0.000000
sglang-agg 0 0.000000 0.000000
trtllm-disagg 0 0.000000 0.000000
sglang-disagg 0 0.000000 0.000000
trtllm-agg 0 0.000000 0.000000
vllm-disagg 0 0.000000 0.000000
MiniMax-M2.5|b300_sxm|vllm|agg 0 0.000000 0.000000
Qwen3.5-397B-A17B|b300_sxm|sglang|agg 0 0.000000 0.000000
Kimi-K2.5|gb200|trtllm|disagg 0 0.000000 0.000000
Kimi-K2.5|b200_sxm|vllm|agg 0 0.000000 0.000000
Kimi-K2.5|b300_sxm|vllm|agg 0 0.000000 0.000000
MiniMax-M2.5|b200_sxm|vllm|agg 0 0.000000 0.000000
DeepSeek-V3|gb200|trtllm|disagg 0 0.000000 0.000000
DeepSeek-V3|gb300|trtllm|disagg 0 0.000000 0.000000
DeepSeek-V3|gb200|sglang|disagg 0 0.000000 0.000000
DeepSeek-V3|gb300|sglang|disagg 0 0.000000 0.000000
DeepSeek-V3|h200_sxm|sglang|agg 0 0.000000 0.000000
DeepSeek-V3|b200_sxm|trtllm|disagg 0 0.000000 0.000000
DeepSeek-V3|h200_sxm|sglang|disagg 0 0.000000 0.000000
MiniMax-M2.5|h200_sxm|vllm|agg 0 0.000000 0.000000
Qwen3.5-397B-A17B|b200_sxm|sglang|agg 0 0.000000 0.000000
DeepSeek-V3|b200_sxm|sglang|disagg 0 0.000000 0.000000
DeepSeek-V3|b200_sxm|sglang|agg 0 0.000000 0.000000
DeepSeek-V3|b200_sxm|trtllm|agg 0 0.000000 0.000000
DeepSeek-V3|h200_sxm|trtllm|disagg 0 0.000000 0.000000
DeepSeek-V3|h100_sxm|trtllm|disagg 0 0.000000 0.000000
DeepSeek-V3|h200_sxm|trtllm|agg 0 0.000000 0.000000
Kimi-K2.5|gb200|vllm|disagg 0 0.000000 0.000000
DeepSeek-V3|b300_sxm|sglang|agg 0 0.000000 0.000000
DeepSeek-R1-NVFP4|b200_sxm|trtllm|agg 0 0.000000 0.000000
DeepSeek-V3.1-NVFP4|b200_sxm|trtllm|agg 0 0.000000 0.000000
Qwen3-32B-NVFP4|b200_sxm|trtllm|disagg 0 0.000000 0.000000
Qwen3-32B-NVFP4|b200_sxm|trtllm|agg 0 0.000000 0.000000
DeepSeek-R1|gb200|trtllm|agg 0 0.000000 0.000000
Qwen3-32B|gb200|trtllm|agg 0 0.000000 0.000000
DeepSeek-V3.1-NVFP4|gb200|trtllm|agg 0 0.000000 0.000000
Qwen3-32B-NVFP4|gb200|trtllm|disagg 0 0.000000 0.000000
Qwen3-32B-NVFP4|gb200|trtllm|agg 0 0.000000 0.000000
Qwen3-32B|h100_sxm|sglang|agg 0 0.000000 0.000000
Qwen3-32B|h100_sxm|sglang|disagg 0 0.000000 0.000000
Qwen3-235B-A22B-FP8|h100_sxm|trtllm|agg 0 0.000000 0.000000
Qwen3-32B|h100_sxm|trtllm|agg 0 0.000000 0.000000
Qwen3-32B|h100_sxm|vllm|agg 0 0.000000 0.000000
Qwen3-235B-A22B-FP8|h200_sxm|sglang|agg 0 0.000000 0.000000
Qwen3-235B-A22B-FP8|h200_sxm|vllm|agg 0 0.000000 0.000000
Llama-3.1-8B-Instruct-FP8|h200_sxm|vllm|agg 0 0.000000 0.000000
MiniMax-M2.5|h100_sxm|vllm|agg 0 0.000000 0.000000
DeepSeek-R1|b200_sxm|sglang|agg 0 0.000000 0.000000
DeepSeek-R1|h200_sxm|sglang|agg 0 0.000000 0.000000
DeepSeek-R1|b200_sxm|trtllm|agg 0 0.000000 0.000000
DeepSeek-R1|h200_sxm|trtllm|agg 0 0.000000 0.000000
Comparison plot AIC accuracy regression testing comparison plot

Artifact bundle: accuracy-regression-testing-results

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant