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
Open
feat(cli): add default values for gpu_vendor and guest_os in build CLI#89coketaste wants to merge 8 commits intocoketaste/refactor-disfrom
coketaste wants to merge 8 commits intocoketaste/refactor-disfrom
Conversation
…mmand Add sensible defaults (AMD/UBUNTU) to simplify build operations for the common use case while maintaining full backward compatibility and explicit override capability.
…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
There was a problem hiding this comment.
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_defaultswith shared defaults + helper to apply them. - Apply defaults in both CLI validation (
validate_additional_context) andBuildOrchestratorinitialization. - 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_vendoris uppercased for validation, but the normalized value is not written back intocontext. Downstream code (notablyContext.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 intocontext(and do the same forguest_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).
leconcio
previously approved these changes
Mar 27, 2026
Contributor
leconcio
left a comment
There was a problem hiding this comment.
I've reviewed and tested this branch, it's indeed backwards compatible with madengineV1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
Benefits:
Testing:
Documentation:
Backward Compatibility:
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"}'