Skip to content

feat(cli): parallel catalog validation and fleet rollup - #123

Open
mimran-khan wants to merge 19 commits into
NVIDIA:mainfrom
mimran-khan:feat/parallel-catalog-workers
Open

feat(cli): parallel catalog validation and fleet rollup#123
mimran-khan wants to merge 19 commits into
NVIDIA:mainfrom
mimran-khan:feat/parallel-catalog-workers

Conversation

@mimran-khan

Copy link
Copy Markdown
Contributor

Fixes #122
Related: #120, #121

This branch includes two catalog features that stack cleanly:

  1. catalog-summary.json ([FEA]: Machine-readable catalog fleet rollup report #120 / feat(cli): write catalog-summary.json after catalog validate #121): fleet rollup after catalog validate
  2. --workers N ([FEA]: Parallel catalog validation with --workers #122): parallel child-process validation for catalogs

--workers defaults to 1 (serial, same per-skill pipeline view as today). Above 1, skills validate in isolated processes with per-skill output dirs under -o. The parent Click context (or sys.argv when launched from the real CLI) is used to rebuild each child validate invocation, so pytest and production entry points both work.

Parallel mode skips the per-skill Rich pipeline view and prints a single fleet scoreboard at the end. Exit code stays nonzero if any skill failed.

If #121 lands first, I can rebase this to workers-only; happy to split either way.

Test plan

  • pytest tests/test_commands.py -k catalog
  • Serial catalog tests still pass with --workers 1 (default)
  • --workers 2 integration test on two fixture skills

Emit a machine-readable fleet rollup at the reports root with per-skill
status, optional severity totals from child JSON reports, and report paths.
Create the output directory when needed so summary writes survive early
skill failures.

Fixes NVIDIA#120

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
Catalog validate accepts --workers N to run skills in isolated child
processes. Values above 1 skip the per-skill pipeline view and rebuild
per-skill argv from the parent Click context or sys.argv.

Fixes NVIDIA#122

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
Comment thread src/skillevaluator/cli.py Outdated
Comment thread src/skillevaluator/cli.py Outdated
Comment thread src/skillevaluator/cli.py Outdated
Comment thread src/skillevaluator/cli.py Outdated
Rebuild child argv without dropping positional catalog paths, track fresh per-skill
JSON reports instead of stale files, write catalog-summary.json atomically, and
fix --include-skills forwarding for context fallback.

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
@mimran-khan

Copy link
Copy Markdown
Contributor Author

Worker argv, stale JSON, atomic summary, and --include-skills fixes are pushed. Ready for re-review.

@chrisknvidia

chrisknvidia commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

@mimran-khan : Re-reviewed at current head a7ea2e7:

  1. Still broken: real CLI worker argv drops option values. _catalog_child_argv_from_sys() now discards every non-flag token, including values such as schema, 95, model names, and paths. A real console run of skillevaluator validate <catalog> --workers 2 --no-llm --no-dedup --checks schema -o <out> exits 1 and reports 0/1 passed. The new CliRunner test passes because it exercises _catalog_child_argv_from_ctx(), not the production sys.argv path.
  2. Still broken: stale JSON is reused when no new JSON is produced. _run_catalog_skill_worker() returns json_report_name=None, but _catalog_skill_entry() treats None as permission to call _latest_skill_json_report(); serial mode also still scans existing output. Reusing -o reproduced current passed: false / reason: validation failed alongside stale overall_passed: true, overall_status: passed, and old severity counts.

The atomic summary writer and --include-skills fixes were independently verified.

Comment thread src/skillevaluator/cli.py Outdated

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Parallel catalog execution still loses real CLI option values, changes explicit CLI-only reporting, and can reuse stale per-skill JSON after a worker produces no report. Focused catalog tests and Ruff/diff checks passed; the single Python 3.12 CI failure is an unrelated flaky NVIDIA bridge socket test. Requesting changes for the reproducible worker-boundary defects in the review threads.

Comment thread src/skillevaluator/cli.py
Rebuild child argv from Click params, preserve -r cli when selected, track
per-run JSON in serial catalog mode, and stop attaching stale reports when
no new JSON was produced this run.

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
Comment thread src/skillevaluator/cli.py Outdated
Comment thread src/skillevaluator/cli.py Outdated
Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
Do not forward the implicit cli default to catalog workers, and ignore
SARIF sidecars when picking the standard JSON report for fleet rollup.

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
Record the JSON artifact emitted by each validate invocation instead of
rediscovering reports via directory diffs, which missed same-second
overwrites in parallel catalog mode. Also stop pre-creating catalog
summary parents through symlinked paths.
Comment thread src/skillevaluator/cli.py Outdated


def _record_validate_json_report(report_name: str | None) -> None:
global _last_validate_json_report

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P1] Remove the module-global report handoff. The required Python 3.12 job deterministically fails here and at line 348 because Ruff reports PLW0603; process-global state also makes nested or re-entrant validate calls share provenance. Please return the emitted JSON basename through the Click invocation and worker result, or store it in invocation-scoped context state, instead of mutating _last_validate_json_report.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The previously reported worker argument, report-format, stale-report, SARIF-selection, and same-second overwrite defects are fixed at the current head, and all 53 focused command tests pass. Required CI is still red: Ruff rejects the new module-global report handoff, and DCO reports fb87d4a and 21cdb16 as unsigned. Please replace the global with invocation-scoped state, fix the sign-offs, and rerun the checks.

rng1995 and others added 3 commits September 12, 2026 00:36
Replace module-global report name slot so parallel catalog workers and
Ruff PLW0603 stay clean while preserving exact JSON binding after validate.

Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
@mimran-khan

Copy link
Copy Markdown
Contributor Author

Same ContextVar handoff as #121 at 184a95c, merged with main. Parallel workers consume the report name from the child validate context without a process-wide slot. Same-second JSON overwrite fix and exact basename binding are unchanged. Ready for re-review when CI runs on your side.

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.

[FEA]: Parallel catalog validation with --workers

3 participants