Skip to content
This repository was archived by the owner on Apr 20, 2026. It is now read-only.

fix: honor explicit name: field in override_* sections - #204

Open
weireweire wants to merge 1 commit into
ishandhanani:mainfrom
weireweire:fix/override-explicit-name
Open

fix: honor explicit name: field in override_* sections#204
weireweire wants to merge 1 commit into
ishandhanani:mainfrom
weireweire:fix/override-explicit-name

Conversation

@weireweire

@weireweire weireweire commented Mar 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • generate_override_configs previously ignored any name: field inside an override_* dict, always overwriting it with an auto-generated {base_name}_{suffix}. Now the auto-generated name is only used as a fallback when no name: is provided — consistent with how zip_override_* already handles named variants.
  • Adds test coverage for the new behavior.

Example

base:
  name: "b200-fp4-mtp-8k1k"
  ...

override_maxtpt_4p1d:
  name: "b200-fp4-max-tpt-dep4-4p-dep8-1d"  # now respected
  ...

Before: dry-run showed b200-fp4-mtp-8k1k_maxtpt_4p1d
After: dry-run shows b200-fp4-max-tpt-dep4-4p-dep8-1d

Test plan

  • make check passes (335 tests)

Summary by CodeRabbit

  • Bug Fixes

    • Override configurations now properly respect explicitly provided names. Previously, user-supplied names were being replaced with auto-generated identifiers. Names are now correctly preserved during configuration merging.
  • Tests

    • Added test coverage verifying that explicit names in override configurations are correctly preserved instead of being replaced.

Previously, generate_override_configs always overwrote the merged name
with an auto-generated "{base_name}_{suffix}", even when the override
dict supplied an explicit name: field. Now the auto-generated name is
only used as a fallback when no name is provided, consistent with how
zip_override_* already handles named variants.

Adds test coverage for the new behavior.
@coderabbitai

coderabbitai Bot commented Mar 4, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

Modified the generate_override_configs function in the config module to preserve explicit names provided in override configurations instead of always auto-generating names based on base name suffixes. Added test coverage validating that explicit names are respected in both standard and selector-based override scenarios.

Changes

Cohort / File(s) Summary
Config Override Naming Logic
src/srtctl/core/config.py
Refined auto-naming behavior to check if override provides an explicit name before applying name derivation; preserves user-supplied names across single override and all overrides expansion code paths.
Override Configuration Tests
tests/test_override.py
Added test_override_explicit_name test case to validate explicit name handling in override configurations, including selector-based override scenarios.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • PR #199: Also modifies generate_override_configs in the same file; this PR refines the auto-naming logic to preserve explicit names introduced in that PR.

Suggested reviewers

  • ishandhanani
  • nlevin-ui

Poem

🐰 A name once forced, now free to choose,
When configs speak, their voice won't lose,
With careful checks, we let them stand,
Explicit names, exactly planned! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: honoring explicit name fields in override sections instead of auto-generating names.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

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

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

🧹 Nitpick comments (1)
src/srtctl/core/config.py (1)

316-321: Consider extracting shared override-merge naming logic into a helper.

The same block appears twice; a helper reduces drift risk and keeps behavior changes centralized.

♻️ Proposed refactor
+def _merge_override_variant(base: dict[str, Any], override_dict: dict[str, Any], suffix: str) -> dict[str, Any]:
+    merged = deep_merge(base, override_dict)
+    if "name" not in override_dict:
+        base_name = base.get("name", "unnamed")
+        merged["name"] = f"{base_name}_{suffix}"
+    return merged
+
 def generate_override_configs(
     raw_config: dict[str, Any],
     selector: str | None = None,
 ) -> list[tuple[str, dict[str, Any]]]:
@@
         suffix = selector[len("override_") :]
         override_dict = raw_config[selector]
-        merged = deep_merge(base, override_dict)
-        if "name" not in override_dict:
-            base_name = base.get("name", "unnamed")
-            merged["name"] = f"{base_name}_{suffix}"
+        merged = _merge_override_variant(base, override_dict, suffix)
         return [(suffix, merged)]
@@
     for key in override_keys:
         suffix = key[len("override_") :]
         override_dict = raw_config[key]
-        merged = deep_merge(base, override_dict)
-        if "name" not in override_dict:
-            base_name = base.get("name", "unnamed")
-            merged["name"] = f"{base_name}_{suffix}"
+        merged = _merge_override_variant(base, override_dict, suffix)
         configs.append((suffix, merged))

Also applies to: 327-331

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/srtctl/core/config.py` around lines 316 - 321, There are two identical
blocks that merge an override into a base and set a default name if missing;
extract this into a small helper (e.g., _merge_override_with_name(base,
override_dict, suffix)) that calls deep_merge(base, override_dict), ensures
merged["name"] is set to f"{base.get('name','unnamed')}_{suffix}" when "name"
not in override_dict, and returns (suffix, merged); then replace the duplicated
blocks in the functions that currently use the inline logic with a call to this
helper to centralize behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/srtctl/core/config.py`:
- Around line 316-321: There are two identical blocks that merge an override
into a base and set a default name if missing; extract this into a small helper
(e.g., _merge_override_with_name(base, override_dict, suffix)) that calls
deep_merge(base, override_dict), ensures merged["name"] is set to
f"{base.get('name','unnamed')}_{suffix}" when "name" not in override_dict, and
returns (suffix, merged); then replace the duplicated blocks in the functions
that currently use the inline logic with a call to this helper to centralize
behavior.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f0eef751-7810-415d-9e5b-a27ee75128f0

📥 Commits

Reviewing files that changed from the base of the PR and between c193a65 and d292fc9.

📒 Files selected for processing (2)
  • src/srtctl/core/config.py
  • tests/test_override.py

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant