Skip to content

feat(cli): add default values for gpu_vendor and guest_os in build CLI#89

Open
coketaste wants to merge 8 commits intocoketaste/refactor-disfrom
coketaste/refactor-dis-refactor-config
Open

feat(cli): add default values for gpu_vendor and guest_os in build CLI#89
coketaste wants to merge 8 commits intocoketaste/refactor-disfrom
coketaste/refactor-dis-refactor-config

Conversation

@coketaste
Copy link
Copy Markdown
Collaborator

feat(cli): add default values for gpu_vendor and guest_os in build command

Add sensible defaults (AMD/UBUNTU) to simplify build operations for the
common use case while maintaining full backward compatibility and explicit
override capability.

Changes:

  • Add DEFAULT_GPU_VENDOR="AMD" and DEFAULT_GUEST_OS="UBUNTU" constants
  • Modify validate_additional_context() to apply defaults when fields missing
  • Display informative console messages when defaults are used
  • Preserve merge priority: file → CLI string → defaults (lowest)

Benefits:

  • Simplified onboarding for AMD/Ubuntu users (majority case)
  • Less verbose commands for development and testing
  • Clear user communication about defaults via Rich console
  • Full customization still available for production deployments

Testing:

  • Add comprehensive test suite (12 test cases) in test_validators.py
  • All tests pass with 100% coverage of default behavior
  • Validates partial overrides, file merging, and error cases

Documentation:

  • Update README.md Quick Start to show optional additional-context
  • Add "Default Configuration Values" section to docs/configuration.md
  • Simplify examples in docs/usage.md and docs/cli-reference.md
  • Include production recommendations for explicit configuration

Backward Compatibility:

  • 100% compatible with existing workflows
  • Explicit configurations work identically
  • Run command unaffected (doesn't call validator)
  • Exit codes unchanged

Example usage:

Before (required)

madengine build --tags dummy --additional-context '{"gpu_vendor": "AMD", "guest_os": "UBUNTU"}'

After (optional for AMD/Ubuntu)

madengine build --tags dummy

Override for other environments

madengine build --tags dummy --additional-context '{"gpu_vendor": "NVIDIA"}'

…mmand

Add sensible defaults (AMD/UBUNTU) to simplify build operations for the
common use case while maintaining full backward compatibility and explicit
override capability.
@coketaste coketaste self-assigned this Mar 26, 2026
…file filtering

validate_additional_context already applied AMD/UBUNTU defaults, but the merged
dict was not passed to BuildOrchestrator, so Context.filter() saw no vendor keys
and could build both AMD and NVIDIA Dockerfiles.

- Add core/additional_context_defaults.py with apply_build_context_defaults()
- Refactor CLI validators to use the shared helper
- build command: pass repr(validated context) and drop duplicate file load
- BuildOrchestrator: apply defaults after merge; build Context from final
  additional_context post-ConfigLoader
- Update unit/integration tests; add tests for defaults helper
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds centralized defaults for build-time additional_context (gpu_vendor=AMD, guest_os=UBUNTU) so common build workflows can omit --additional-context, while keeping override support and updating docs/tests accordingly.

Changes:

  • Introduce madengine.core.additional_context_defaults with shared defaults + helper to apply them.
  • Apply defaults in both CLI validation (validate_additional_context) and BuildOrchestrator initialization.
  • Update build command wiring, tests, and documentation to reflect defaulting behavior.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
src/madengine/core/additional_context_defaults.py Adds shared defaults/constants and a helper to apply them.
src/madengine/cli/validators.py Applies defaults during validation and prints informational messages.
src/madengine/orchestration/build_orchestrator.py Ensures orchestrator has defaults so Dockerfile filtering can work without explicit context.
src/madengine/cli/commands/build.py Uses validated+merged context (and reuses it for batch manifest processing).
tests/unit/test_validators.py New validator-focused tests for defaulting/merging behavior.
tests/unit/test_additional_context_defaults.py New tests for the shared defaults helper.
tests/unit/test_orchestration.py Updates expectations for BuildOrchestrator default context.
tests/integration/test_orchestrator_workflows.py Updates integration expectations for orchestrator defaults.
docs/usage.md Updates quick start examples and notes about defaults.
docs/configuration.md Documents default values and when they apply.
docs/cli-reference.md Documents build defaults and messaging.
README.md Updates quick start examples and notes about defaults.
Comments suppressed due to low confidence (1)

src/madengine/cli/validators.py:96

  • gpu_vendor is uppercased for validation, but the normalized value is not written back into context. Downstream code (notably Context.filter() which uses exact string equality) can fail to match Dockerfile contexts if the user passes lowercase values (e.g., "amd"). After validating, persist the canonical value back into context (and do the same for guest_os).
    # Validate gpu_vendor
    gpu_vendor = context["gpu_vendor"].upper()
    if gpu_vendor not in VALID_GPU_VENDORS:
        console.print(f"❌ Invalid gpu_vendor: [red]{context['gpu_vendor']}[/red]")

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Add structural checks for --additional-context (nested types, known keys)
with clear typer.Exit messages and examples. Unify run merge/validate with
build when context is non-empty.

Remove the duplicate MAD_SYSTEM_GPU_ARCHITECTURE info line from build-only
Context init. After model discovery, warn only when a selected Dockerfile
needs ARG MAD_SYSTEM_GPU_ARCHITECTURE without a default and the effective
arch cannot be resolved (context or Dockerfile).

Add dockerfile_requires_explicit_mad_arch_build_arg helper, drop duplicate
DockerBuilder methods, and extend unit tests (validators, CLI, Dockerfile
heuristic, orchestrator warning, context messages).
…dators

- After validation, write canonical uppercase gpu_vendor and guest_os into the
  context dict so Context.filter() matches exactly.
- Align test_validate_additional_context_case_insensitive with that behavior.
- Drop unused imports: Panel in validators.py; patch/MagicMock in
  test_validators.py (flake8 F401).
@ROCm ROCm deleted a comment from Copilot AI Mar 27, 2026
leconcio
leconcio previously approved these changes Mar 27, 2026
Copy link
Copy Markdown
Contributor

@leconcio leconcio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reviewed and tested this branch, it's indeed backwards compatible with madengineV1

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.

3 participants