Add validate command for custom training#7407
Add validate command for custom training#7407saanikaguptamicrosoft wants to merge 23 commits intoAzure:foundry-training-devfrom
Conversation
jongio
left a comment
There was a problem hiding this comment.
PR Review - #7407
Add validate command for custom training by @saanikaguptamicrosoft
Summary
What: Adds an offline job validate command that checks a YAML job definition for required fields, invalid git paths, local path existence, placeholder correctness, and empty input/output definitions - collecting all findings before reporting instead of failing on the first error.
Why: Lets users catch YAML mistakes locally before submitting to the backend, saving a round-trip.
Assessment: The validation logic is solid and well-structured, but the command can't actually run offline due to the parent command's PersistentPreRunE hook. This should be addressed before merge. A few edge cases in the validator produce confusing output (duplicate errors on git paths, silent pass on ${inputs.xxx} typos).
Findings
| Category | High | Medium | Low |
|---|---|---|---|
| Design | 1 | 0 | 0 |
| Logic | 0 | 2 | 0 |
| Error Handling | 0 | 1 | 0 |
| Consistency | 0 | 1 | 1 |
| Tests | 0 | 0 | 1 |
| Total | 1 | 4 | 2 |
Key Findings
- [HIGH] Design: Parent
jobcommand'sPersistentPreRunErequires Azure env setup, preventing offline use - [MEDIUM] Logic:
git://code paths produce duplicate errors (git-not-supported + local-path-not-found) - [MEDIUM] Logic:
${inputs.xxx}typos silently bypass single-brace detection - [MEDIUM] Error Handling:
os.Statpermission/IO errors silently ignored in path checks - [MEDIUM] Consistency: Missing
-fshorthand for--fileflag (inconsistent withjob submit) - [LOW] Consistency: Missing
Longdescription on cobra command (all sibling commands have one) - [LOW] Tests: No test for the
git://double-error scenario (assert only 1 finding, not 2)
Test Coverage Estimate
- Well covered: required fields, git path detection, local path checks, placeholder mapping, single-brace detection, empty definitions, multiline commands
- Missing: command-layer test (cobra handler),
${inputs.xxx}edge case,git://double-error assertion,os.Statpermission errors, empty/zero-byte YAML input
What's Done Well
- Collects all findings instead of failing on the first error - much better UX than the existing
ValidateJobDefinition - Good separation between the command handler and validation logic
- Optional
[...]block handling for input placeholders is a nice touch - Test file has clear YAML comments showing what each test case maps to
5 inline comments below.
cli/azd/extensions/azure.ai.customtraining/internal/utils/job_validator.go
Show resolved
Hide resolved
cli/azd/extensions/azure.ai.customtraining/internal/utils/job_validator.go
Show resolved
Hide resolved
cli/azd/extensions/azure.ai.customtraining/internal/utils/job_validator.go
Show resolved
Hide resolved
jongio
left a comment
There was a problem hiding this comment.
Previous 5 findings addressed. New issues found:
- job_validator_test.go:223 - single-brace test assertions search for
"single-brace '...'"but the actual message is"Incorrect placeholder format...". Three assertions will fail (223, 226, 236) - job_validator.go:201 - duplicate findings when the same placeholder key appears twice in command
- job_validator.go:106 - fmt.Sprintf with no format verbs (staticcheck SA1006)
| job := validJob() | ||
| job.Command = "python train.py --data {inputs.training_data} --out {outputs.model}" | ||
| result := ValidateJobOffline(job, ".") | ||
| if f := findFindingByMessage(result, "single-brace '{inputs.training_data}'"); f == nil || f.Severity != SeverityError { |
There was a problem hiding this comment.
[HIGH] These test assertions don't match the actual error message.
validateSingleBracePlaceholders produces "Incorrect placeholder format - use '${{inputs.training_data}}' instead" but these 3 assertions (lines 223, 226, 236) search for "single-brace '...'". They'll fail because that substring isn't in the message. Line 249's negative check also passes vacuously.
Fix: match on the actual message, e.g. search for "Incorrect placeholder format" or "${{inputs.training_data}}" instead.
| command := job.Command | ||
|
|
||
| // Find all ${{inputs.xxx}} and ${{outputs.xxx}} references | ||
| for _, match := range placeholderRegex.FindAllStringSubmatch(command, -1) { |
There was a problem hiding this comment.
[MEDIUM] validatePlaceholders (and validateInputOutputDefinitions at L255) iterates ALL regex matches. If the same key appears twice in the command, you get duplicate identical findings. Track seen keys with a map to dedup.
| result.Findings = append(result.Findings, ValidationFinding{ | ||
| Field: "code", | ||
| Severity: SeverityError, | ||
| Message: fmt.Sprintf("git paths are not supported"), |
There was a problem hiding this comment.
[LOW] fmt.Sprintf with no format verbs - just use a plain string literal.
Notes
Added validation for-
git://orgit+codeandinputs.<key>.pathare existent${{inputs.xxx}}, thexxxshould be present as key in inputs.{}orNone, throw error.${{outputs.xxx}}, thexxxmay or may not be present as key in inputs. If it's not present, means user wants to go with default output values. If it's present means user wants to override output configs.{}orNone, show warning. Because user might have forgotten to override the value.${{inputs.xxx}}is wrapped inside[ ], it's optional, so we don't validate its presence in inputs key.{inputs.xxx}or{outputs.xxx}or${inputs.xxx}or${outputs.xxx}should throw errorAdded unit tests.
Testing