Add CLI for running and validating checks#2377
Add CLI for running and validating checks#2377raghavtries wants to merge 5 commits intoGiskard-AI:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a command-line interface for Giskard, enabling users to run, validate, and list checks using YAML or JSON definition files. The implementation includes a new giskard entry point, logic for dynamic Python target resolution, and support for multiple output formats like JSON and JUnit. Review feedback suggests simplifying the attribute resolution logic, ensuring parent directories are created before writing output files, and avoiding reliance on private internal registries for check discovery.
| if path is None: | ||
| print(output) | ||
| return | ||
| path.write_text(output, encoding="utf-8") |
There was a problem hiding this comment.
The path.write_text call will fail if the parent directory of the output file does not exist. For a better user experience, consider ensuring the parent directory exists before writing.
| path.write_text(output, encoding="utf-8") | |
| path.parent.mkdir(parents=True, exist_ok=True) | |
| path.write_text(output, encoding="utf-8") |
kevinmessiaen
left a comment
There was a problem hiding this comment.
@raghavtries Thanks for your submission. This look good but I feel like we could simplify a bit by leveraging Pydantic capabilities
|
Thanks for the details in the review, I addressed the feedback by moving scenario target, suite target, trace_type loading into Pydantic validators, along with simplifying CLI loading through TypeAdapter, keeping the serialized format canonical with Also made YAML support optional and moved reusable CLI definitions into fixture files. I also fixed output directory creation for |
|
@gemini-code-assist Can you review the updated code? |
There was a problem hiding this comment.
Code Review
This pull request introduces a command-line interface for Giskard checks, enabling the execution, validation, and listing of scenarios and suites from JSON or YAML definitions. Key additions include a robust Python reference resolution system, Pydantic-based validation for dynamic targets, and support for various output formats like JUnit and JSON. Review feedback highlighted the need to prevent over-aggressive clearing of the module cache during reference resolution and to ensure parent directories are created when writing JUnit reports.
| if _module_exists_in_search_path(module_name, preferred_search_path): | ||
| if not _module_belongs_to_search_path( | ||
| cached_module, preferred_search_path | ||
| ): | ||
| sys.modules.pop(module_name, None) | ||
| elif not _module_belongs_to_search_paths(cached_module, search_paths): | ||
| sys.modules.pop(module_name, None) |
There was a problem hiding this comment.
The logic for clearing sys.modules is too aggressive. It currently removes any cached module that is not found in the provided search paths (e.g., standard library modules like os or json if they are referenced in a python:... string). This forces unnecessary re-imports and can lead to unexpected side effects in long-running processes. The cache should only be cleared if a collision is detected with a module in the preferred search path (the definition file's directory).
| if _module_exists_in_search_path(module_name, preferred_search_path): | |
| if not _module_belongs_to_search_path( | |
| cached_module, preferred_search_path | |
| ): | |
| sys.modules.pop(module_name, None) | |
| elif not _module_belongs_to_search_paths(cached_module, search_paths): | |
| sys.modules.pop(module_name, None) | |
| if _module_exists_in_search_path(module_name, preferred_search_path): | |
| if not _module_belongs_to_search_path( | |
| cached_module, preferred_search_path | |
| ): | |
| sys.modules.pop(module_name, None) |
| elif args.format == "junit": | ||
| xml = _build_suite_result(execution).to_junit_xml(path=output_path) | ||
| if output_path is None: | ||
| print(xml) |
There was a problem hiding this comment.
When using the junit format with an --output path, the parent directories are not automatically created. This can cause the command to fail if the specified output path includes directories that do not yet exist. For consistency with the json format (which uses _write_text_output), ensure the parent directory is created before calling to_junit_xml.
| elif args.format == "junit": | |
| xml = _build_suite_result(execution).to_junit_xml(path=output_path) | |
| if output_path is None: | |
| print(xml) | |
| elif args.format == "junit": | |
| if output_path: | |
| output_path.parent.mkdir(parents=True, exist_ok=True) | |
| xml = _build_suite_result(execution).to_junit_xml(path=output_path) | |
| if output_path is None: | |
| print(xml) |
Signed-off-by: Raghav Sub <theraghavs06@gmail.com>
dc03c14 to
657d953
Compare
|
Hi there! I've made the needed edits from the Gemini review, please let me know if there's anything else I should change! |
Description
Add a
giskardCLI for running and validating serialized scenarios and suites.This PR adds:
giskard run <file>for YAML/JSON scenario and suite definitionsgiskard validate <file>to validate definitions without executiongiskard list checksto expose built-in check kindsIt also preserves support for serialized
trace_typevalues and ensurespython:...target resolution prefers the definition file directory over the current working directory when module names collide.Related Issue
Closes #2376
Type of Change
Checklist
CODE_OF_CONDUCT.mddocument.CONTRIBUTING.mdguide.uv.lockrunninguv lock(only applicable whenpyproject.tomlhas been modified)