training: Align training job yaml schema with new command job schema - #9349
training: Align training job yaml schema with new command job schema#9349saanikaguptamicrosoft wants to merge 2 commits into
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 21 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Aligns training-job YAML, wire models, display logic, and scaffolding with Foundry’s capacity-unit submission schema.
Changes:
- Replaces GPU sizing with top-level capacity units and priority.
- Restricts resource configuration to instance count.
- Updates job display fallbacks and generated YAML.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/models/job.go |
Adds new command-job wire fields. |
pkg/models/history.go |
Supports new and legacy compute metadata. |
pkg/models/common.go |
Narrows resource configuration. |
internal/utils/yaml_parser.go |
Updates the accepted YAML shape. |
internal/cmd/job_submit.go |
Maps new fields into submissions. |
internal/cmd/job_show.go |
Updates compute-detail rendering. |
internal/cmd/init_template.go |
Scaffolds the new YAML format. |
Comments suppressed due to low confidence (1)
cli/azd/extensions/azure.ai.training/internal/cmd/job_show.go:483
- [azd-code-reviewer] These top-level Get Job fields are still rendered only inside the
History.Compute != nilbranch. Run history is supplementary and may be unavailable, so a new job can show its compute name while omitting bothCapacity UnitsandPriority. Render top-level values independently, then consult run history only for fallback values.
// Prefer the user-submitted capacityUnitCount (from Get Job) when present — for
// partial-SKU jobs it reflects the actual allocation. Fall back to the SKU GPU
// count reported by run history for older jobs that pre-date capacityUnitCount.
if d.Job.Properties.CapacityUnitCount > 0 {
fmt.Fprintf(w, "Capacity Units:\t%d\n", d.Job.Properties.CapacityUnitCount)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
cli/azd/extensions/azure.ai.training/internal/utils/yaml_parser.go:85
- [azd-code-reviewer]
yaml.Unmarshalignores unknown keys, so removing these fields makes existing files containinggpu_count,resources.instance_type,shm_size,docker_args, orpropertiesvalidate successfully while silently discarding their allocation settings. Reject removed keys with migration guidance (for example,gpu_count→capacity_unit_count) in both submit and offline-validation parsing paths, or enable strict known-field decoding consistently.
// Only instance_count is honored; instance_type, slaTier, priority and the AISuperComputer
// properties block have been removed — the service now infers the SKU from the compute
// cluster, priority is a top-level job field, and users specify partial capacity via the
// top-level capacity_unit_count field.
type ResourceDefinition struct {
cli/azd/extensions/azure.ai.training/pkg/models/job.go:27
- [azd-code-reviewer] Removing
GPUCounthere dropsgpuCountwhileGetJobdecodes older responses. JSON mode then re-marshals this typed model without fetching run history (job_show.go:83-92), so its documented backward-compatible “raw job response” silently loses the legacy field. KeepGPUCounton the read model; its zero value remains omitted from new submit payloads.
CapacityUnitCount int `json:"capacityUnitCount,omitempty"`
Priority string `json:"priority,omitempty"`
cli/azd/extensions/azure.ai.training/internal/utils/yaml_parser.go:33
- [azd-code-reviewer] The generated template documents constrained values for both new fields, but offline validation currently accepts any integer/string and also allows
capacity_unit_counttogether withinstance_count, despite describing them as alternatives. Invalid values therefore passjob validateand fail only at submission. Add validator rules and table-driven tests for the capacity-unit set, the priority enum, and mutually exclusive sizing options.
CapacityUnitCount int `yaml:"capacity_unit_count"`
Priority string `yaml:"priority"`
cli/azd/extensions/azure.ai.training/internal/cmd/job_show.go:488
- [azd-code-reviewer] These top-level Get Job fields are still inside the
History.Compute != nilbranch. If supplementary run-history retrieval fails or returns no compute object,CapacityUnitCountandPriorityare available ond.Jobbut disappear from table output. Read history into an optional fallback, then render the top-level values independently.
// Capacity Units: prefer the top-level CapacityUnitCount from Get Job,
// then whatever run history reports, then the SKU-level GPU count.
if d.Job.Properties.CapacityUnitCount > 0 {
fmt.Fprintf(w, "Capacity Units:\t%d\n", d.Job.Properties.CapacityUnitCount)
} else if c.CapacityUnitCount > 0 {
Jon Gallant (jongio)
left a comment
There was a problem hiding this comment.
Three things worth addressing before this merges.
Medium
- Silent migration gap:
resources.properties.AISuperComputer.priorityandresources.instance_typeare dropped without warning for any job.yaml scaffolded by 0.0.2-preview. Inline onyaml_parser.go. capacity_unit_countandpriorityskip offline validation even though the new template documents allowed values for both.
Low
RunHistoryCompute.InstanceTypeis retained per the PR description but never rendered.
The init_template.go sizing conflict and the gpu_count migration point already raised on this PR both still look open.
| ShmSize string `yaml:"shm_size"` | ||
| DockerArgs string `yaml:"docker_args"` | ||
| Properties map[string]any `yaml:"properties"` | ||
| InstanceCount int `yaml:"instance_count"` |
There was a problem hiding this comment.
The previous init template scaffolded this block with instance_type: <user to add> and properties.AISuperComputer.slaTier / priority: <user to add>, so every job.yaml generated by 0.0.2-preview has those keys filled in with values the user deliberately picked.
ParseJobFile calls yaml.Unmarshal rather than a decoder with KnownFields(true), so after this change those keys still parse and then get dropped on the floor. The user's priority selection silently reverts to the service default because the new top-level priority is empty, and nothing in the submit output tells them their setting was ignored.
Worth keeping a deprecated shadow of the old fields long enough to emit a migration error:
- keep
InstanceTypeandPropertiesonResourceDefinition - in
ValidateJobOffline, fail when either is set, with something like "resources.instance_type is no longer supported, the service infers the SKU from the compute cluster" and "resources.properties.AISuperComputer.priority moved to the top-level priority field"
That turns a silent behavior change into a one-time actionable error.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
cli/azd/extensions/azure.ai.training/internal/utils/yaml_parser.go:33
- The advertised YAML migration is not implemented here:
JobDefinitionstill acceptsgpu_countand has nocapacity_unit_count. Sinceyaml.Unmarshalignores unknown keys, a file using the new field silently submits zero;buildJobResourceandCommandJobalso continue emittinggpuCount, while the template/show paths still use GPU count. Replace the submit path withCapacityUnitCount(capacity_unit_countin YAML andcapacityUnitCounton the wire), retainingGPUCountonly inRunHistoryComputefor legacy reads.
Priority string `yaml:"priority"`
cli/azd/extensions/azure.ai.training/internal/cmd/job_show.go:497
- Top-level priority is only printed when run history contains a compute object.
fetchJobDetailstolerates a missing/failed run-history lookup, so a validPriorityreturned by Get Job disappears in that case instead of being preferred. Close the history-compute block before this fallback and consult run history only when top-level priority is empty.
// Priority: prefer the top-level Priority from Get Job, otherwise use
// whatever run history reports.
if d.Job.Properties.Priority != "" {
fmt.Fprintf(w, "Priority:\t%s\n", d.Job.Properties.Priority)
} else if c.Priority != "" {
fmt.Fprintf(w, "Priority:\t%s\n", c.Priority)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
cli/azd/extensions/azure.ai.training/internal/utils/yaml_parser.go:85
- [azd-code-reviewer] Removing these fields from the struct does not reject the old schema: both submit and validate use
yaml.Unmarshal, which silently ignores unknown keys. Existing files withresources.instance_type,resources.properties.AISuperComputer.priority, and similar settings will still validate and submit while those settings are discarded. Decode withKnownFields(true)in both paths, or retain deprecated fields long enough to emit a targeted migration error, and update the command-job guide.
// Only instance_count is honored; instance_type, slaTier, priority and the AISuperComputer
// properties block have been removed — the service now infers the SKU from the compute
// cluster, priority is a top-level job field, and users specify partial GPU allocations
// via the top-level gpu_count field (GPU clusters only).
type ResourceDefinition struct {
cli/azd/extensions/azure.ai.training/internal/cmd/job_show.go:493
- [azd-code-reviewer] Top-level
GPUCountandPriorityare still printed only when run-history compute data exists.fetchJobDetailstreats history errors and 404s as nonfatal, sojob showomits these authoritative Get Job values whenever history is unavailable. Move the top-level checks outside this block and use history only as the fallback.
// Priority: prefer the top-level Priority from Get Job, otherwise use
// whatever run history reports.
if d.Job.Properties.Priority != "" {
fmt.Fprintf(w, "Priority:\t%s\n", d.Job.Properties.Priority)
…block Replace the historical capacity_unit_count/gpu_count/nested-priority sizing knobs with the current MFE contract: top-level gpuCount for partial-GPU allocation on GPU clusters, top-level priority, and instance_count as the only field on the resources block. Update job show to render GPUs and inferred instance_type/priority from the run history.
f5d97c7 to
85aefb3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (3)
cli/azd/extensions/azure.ai.training/internal/utils/yaml_parser.go:87
- [azd-code-reviewer] Removing these fields does not make old YAML invalid:
ParseJobFileusesyaml.Unmarshal, which silently ignores unknown mapping keys. Existing files withresources.priority,instance_type, orpropertieswill therefore submit successfully after those settings have been discarded; in particular, priority silently falls back to the service default. Retain the legacy keys long enough to return a migration error, or add strict/targeted decoding that rejects them with the new top-level replacements.
InstanceCount int `yaml:"instance_count"`
cli/azd/extensions/azure.ai.training/internal/cmd/job_submit.go:184
- [azd-code-reviewer] The new YAML-to-wire priority contract is not covered by the existing
buildJobResourcetest, so a wrong nesting or JSON key could regress unnoticed. Add a table-driven case that marshals the payload and verifiespriorityis top-level, along with the CPU/GPU/resource combinations listed in the PR testing notes.
Priority: def.Priority,
cli/azd/extensions/azure.ai.training/internal/cmd/job_show.go:485
- [azd-code-reviewer] These top-level values are still emitted only inside the
History.Compute != nilbranch.fetchJobDetailsdeliberately treats missing/failed run history as nonfatal, sojob showcan have validGPUCountandPriorityfrom Get Job but omit both from the display. Keep only the history fallbacks inside that condition and print the top-level values independently; cover the no-history case.
// GPUs: prefer the top-level GPUCount from Get Job, otherwise use what run history reports.
if d.Job.Properties.GPUCount > 0 {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
cli/azd/extensions/azure.ai.training/internal/cmd/job_submit.go:184
- azd-code-reviewer: Add table-driven tests for the CPU, priority, and partial-GPU submission shapes described by this PR. The current
buildJobResourcetest only checks outputs, so it does not verify thatpriorityandgpuCountare serialized at the intended level or that onlyinstanceCountremains underresources.
GPUCount: def.GPUCount,
Priority: def.Priority,
cli/azd/extensions/azure.ai.training/internal/cmd/job_show.go:492
- azd-code-reviewer: Both top-level values are still rendered only when run history contains a compute object.
GetRunHistorylegitimately returns nil on 404, so a queued or newly submitted job can havegpuCount/priorityin Get Job but omit both from table output. Render the top-level values independently and use run history only as the fallback.
// Priority: prefer the top-level Priority from Get Job, otherwise use
// whatever run history reports.
if d.Job.Properties.Priority != "" {
cli/azd/extensions/azure.ai.training/internal/cmd/job_show.go:399
- azd-code-reviewer: The resource section no longer prints
ShmSize, even thoughResourceConfigstill retains it specifically for Get Job responses. Existing jobs or service-inferred responses that includeshmSizetherefore lose information in table output. Keep the previous “Shared Memory” row on the read path.
fmt.Fprintf(w, "Instance Type:\t%s\n", props.Resources.InstanceType)
| ShmSize string `yaml:"shm_size"` | ||
| DockerArgs string `yaml:"docker_args"` | ||
| Properties map[string]any `yaml:"properties"` | ||
| InstanceCount int `yaml:"instance_count"` |
|
/check-enforcer override |
Notes
Testing