Follow up work for our vally workflows - cleanup, nicer error messages, and handling some edge cases - #9378
Draft
richardpark-msft wants to merge 2 commits into
Draft
Follow up work for our vally workflows - cleanup, nicer error messages, and handling some edge cases#9378richardpark-msft wants to merge 2 commits into
richardpark-msft wants to merge 2 commits into
Conversation
…ad of just using t.Fatal() and friends. - The TotalTrials count field isn't filled out when you run a trial with a single run, so we just default to '1', which is the right answer when there isn't a value set. - Print a message for users that try to submit fork PRs. They won't work because the GH_TOKEN won't have copilot request privileges in forks, so they have to create their branch in our repo instead. - Make qna multi-run, no reason it has to constrain itself to a single run
|
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. |
Contributor
There was a problem hiding this comment.
Pull request overview
Improves Vally eval reliability, reporting, tests, and fork-PR handling.
Changes:
- Defaults missing single-run trial metadata and adds coverage.
- Migrates report tests to
testify. - Expands eval guidance and adds fork-PR rejection.
Show a summary per file
| File | Description |
|---|---|
.github/actions/vally-eval/action.yml |
Adds fork rejection and result propagation. |
cli/azd/test/evals/eval-azd-qna.yaml |
Increases Q&A trials to three. |
cli/azd/test/evals/go.mod |
Adds testify. |
cli/azd/test/evals/go.sum |
Records dependency checksums. |
cli/azd/test/evals/skills/azd/SKILL.md |
Documents complete environment cleanup. |
cli/azd/test/evals/testdata/single-run-results.jsonl |
Adds a single-run fixture. |
cli/azd/test/evals/vally_report.go |
Defaults missing trial totals to one. |
cli/azd/test/evals/vally_report_test.go |
Uses require and tests single-run reporting. |
Review details
- Files reviewed: 7/8 changed files
- Comments generated: 3
- Review effort level: Balanced
…rying to make it a bit clearer which steps run when copilot isn't enabled in a more clean way (hopefully)
Contributor
Reviewed the PR changes. Everything looks good:
Tests: All pass ( |
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (1)
.github/actions/vally-eval/action.yml:160
- azd-code-reviewer: This still gates only on Vally's process exit code, so it does not implement #9340's required per-stimulus/JUnit gating. The linked run exited 0 even with a stimulus failing 0/3 because the eval-wide average passed; the JUnit file recorded three failures, but this step never reads it. Check the generated JUnit/results data and exit nonzero when any stimulus has no passing trials (while retaining the process-exit check).
if [[ "$VALLY_EXIT_CODE" -ne 0 ]]; then
echo "::error::npx vally eval failed with exit code $VALLY_EXIT_CODE -- see the 'Run vally eval' log above."
- Files reviewed: 7/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Contributor
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (1)
.github/actions/vally-eval/action.yml:161
- [azd-code-reviewer] This still treats Vally's process exit code as the complete pass/fail gate. Issue #9340 documents that Vally can return 0 when the eval-wide average passes even though one stimulus is 0/N and the JUnit output contains failures, so those regressions will still pass CI. Parse the generated JUnit or trial results and fail when any stimulus fails before accepting exit code 0.
if [[ "$VALLY_EXIT_CODE" -ne 0 ]]; then
echo "::error::npx vally eval failed with exit code $VALLY_EXIT_CODE -- see the 'Run vally eval' log above."
exit 1
- Files reviewed: 7/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
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.
There were some opportunities to make the workflows a bit nicer, and to tighten up some of our behavior with reporting. Nothing huge, but nice to address and they're pretty simple fixes:
Fixes #9200
Fixes #9340