Skip to content

feat(validation): validate several files per run and look up the schema via OSSIE_SCHEMA - #545

Merged
jbonofre merged 2 commits into
apache:mainfrom
kayemkim:validation/validate-multi-file
Oct 10, 2026
Merged

jbonofre merged 2 commits into
apache:mainfrom
kayemkim:validation/validate-multi-file

Conversation

@kayemkim

@kayemkim kayemkim commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Summary

validate.py took exactly one file. It now accepts any number of paths, reports each one, prints a summary when there is more than one and exits non-zero if any file failed, so validate.py models/*.yaml works in a CI step without a shell loop. Output for a single file is unchanged, which keeps the converter tests that read it (NVIDIA, Cube) as they are.

The default schema path assumed this repository's layout. --schema still wins; otherwise OSSIE_SCHEMA is consulted before the repository default, and the not-found message now says how to point at a schema. I left out the ./schema/ fallback the issue floated, since the environment variable covers the vendored case without inventing a directory convention.

The check sequence itself moved out of main() into validate_document(data, schema), so a caller with a parsed document can run every check, including the ontology checks from #489, without going through the CLI.

This sits on top of #534 and #542. The schema is still read and checked once, before the loop, now in load_schema_or_report. The model side now goes through read_text_or_report, which prints the same messages #534 added but returns None instead of exiting, so an unreadable file among several fails only itself and the run continues. There is a test for that case alongside the other eleven in validation/tests. #271 changes the reporting lines that moved into validate_file unchanged, so whichever lands second is a small rebase.

Verification

  • validation/tests/: 176 passed, validation/test_validate.py: 53 OK, and the full validation workflow (3.11 to 3.14) run locally on this branch merged into current main.
  • Single-file output for examples/tpcds_semantic_model.yaml is byte-identical to main; the NVIDIA and Cube converter suites, which call or import validate.py, pass unchanged.
  • A run over a valid file, a binary file, a missing path and a directory reports each one and ends with 1 of 4 file(s) passed, exit 1.

Related Issues

Follow-up to #266, items 2 and 3 (item 1 was #390).

Checklist

Validation

  • New validation cases are covered by tests

Tests

  • All existing tests pass (pytest / CI green)
  • New functionality is covered by tests

Compliance

  • ASF license headers are present on all new source files (no new files)
  • No third-party dependencies are added without PMC/IPMC approval

🤖 Generated with Claude Code

…ma via OSSIE_SCHEMA

validate.py took exactly one file, so a repository with more than one
model had to wrap it in a shell loop and collect exit codes by hand. It
now accepts any number of paths, reports each one, prints a summary when
there is more than one and exits non-zero if any of them failed. The
output for a single file is unchanged.

The default schema path assumed this repository's layout, which gives a
confusing first error when the script is vendored elsewhere. --schema
still wins; otherwise OSSIE_SCHEMA is consulted before the repository
default, and the not-found message now says how to point at a schema.

The check sequence moves out of main() into validate_document(), so the
checks can run on a parsed document without going through the CLI.

read_text_or_exit becomes read_text_or_report, returning None instead of
exiting, so an unreadable model fails only its own file and the run
continues with the next one. Reading and checking the schema moves into
load_schema_or_report with the same shape; a bad schema still ends the run.

Generated-by: Claude Code
@jbonofre
jbonofre self-requested a review October 9, 2026 07:37
jbonofre
jbonofre previously approved these changes Oct 9, 2026

@jbonofre jbonofre left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Thanks for the multi-file support @kayemkim!

A few minor comments:

  • Only YAML parse errors and RecursionError are caught per file. Any other exception (e.g. a TypeError or KeyError on unusual input) aborts the whole run with a traceback, so remaining file are not validated and no summary prints. That conflicts a bit with the docstring's "every file is validated and reported". I would like to see a per-file catch-all that reports the failure and continues, maybe in this PR.
  • The PASSED line prints only yaml_path.name, so a/model.yaml and b/model.yaml look identical, and warning/error lines carry no path. Printing the full name would make CI logs easier to attribute.
  • Any starting starting with - is now reject as an unknown option, and there's no -- separator. Files like -model.yaml used to validate. Unlikely to matter in practice, but a -- separator would cover it.

@jbonofre

jbonofre commented Oct 9, 2026

Copy link
Copy Markdown
Member

@kayemkim do you want to tackle first comment in this PR or as follow-up?

…after --

A check that raises on one document aborted the whole run with a
traceback, so the files after it were never validated and no summary
printed. validate_file now reports the exception on stdout like any other
failure, keeps the traceback on stderr and returns False, so the run
continues and the exit status still reflects it.

Any argument starting with a dash was rejected as an option, which locked
out a file name such as -model.yaml. A -- separator takes the rest of the
arguments as file names.

Both from review.

Generated-by: Claude Code
@kayemkim

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I took the first and third in this PR, pushed as a second commit so the diff on top of what you approved stays small.

A check that raises now prints Error: <ExceptionType> while validating <path>: <message> on stdout, keeps the traceback on stderr and counts the file as failed, so the remaining files still run and the summary prints. A check raising on a document is a validator bug rather than a model problem, so I did not want to lose the traceback entirely; stderr keeps it out of the per-file report. There is a test that makes validate_document raise on one of two files.

-- now takes the rest of the arguments as file names, with a test for -model.json, and the usage line mentions it.

On the PASSED line: in a multi-file run every file's block starts with == <path> as given on the command line, so a/model.yaml and b/model.yaml are told apart by the header above their PASSED or error lines. I left the single-file output alone because the NVIDIA tests and validation/test_validate.py read it, and a vendored copy may have a grep on the same text. If you would rather have the full path on the PASSED line in the multi-file case anyway, it is a one-line change and I am happy to add it.

@jbonofre jbonofre left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

Two small behavior changes in valiidation/validate.py which are completely fine, but worth being aware of:

  • any argument starting with - is now treated as an unknown option, so a file named -foo.yml now prints usage and exits 1. The new docstring points to -- as the workaround, which is reasonable.
  • a missing schema is now reported before any input file is checked, so the order of error messages differs from before. Exit codes are unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants