feat: return status depending on the checker's severity#216
Conversation
Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
github-actions
Bot
added
documentation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a severity level system for sanity checkers, enabling differentiation between errors and warnings. The implementation adds a Severity enum and updates the Checker base class to support severity-based reporting, where warnings produce WARNING status instead of FAILURE. Several structure validation rules (STR001, STR004-006, STR008-009) are downgraded from errors to warnings.
Key changes:
- Added
Severityenum withERRORandWARNINGlevels - Updated all checkers to explicitly declare their severity level
- Modified reporting to display warnings separately from failures
- Downgraded optional directory/file checks to warnings
Reviewed Changes
Copilot reviewed 56 out of 56 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| t4_devkit/sanity/checker.py | Added Severity enum and updated Checker class to route warnings vs failures based on severity |
| t4_devkit/sanity/result.py | Added WARNING status, make_warning() function, and updated report display/summary logic |
| t4_devkit/sanity/structure/str*.py | Updated structure checkers with Severity import and explicit severity declarations |
| t4_devkit/sanity/format/fmt*.py | Updated format checkers to include Severity import and severity = Severity.ERROR |
| t4_devkit/sanity/reference/ref*.py | Updated reference checkers with severity declarations |
| t4_devkit/sanity/record/rec*.py | Updated record checkers with severity declarations |
| t4_devkit/sanity/tier4/tiv001.py | Added severity declaration and reordered imports |
| t4_devkit/sanity/*/base.py | Updated docstrings to document new severity attribute |
| docs/schema/requirement.md | Changed STR004, STR005, STR006 from Error to Warn |
| docs/cli/t4sanity.md | Updated documentation to show warnings column and new JSON format |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| +-----------+---------+---------+-------+---------+----------+-------+ | ||
| | DatasetID | Version | Status | Rules | Success | Failures | Skips | | ||
| +-----------+---------+---------+-------+---------+----------+-------+ | ||
| | dataset1 | 0 | SUCCESS | 44 | 44 | 0 | 0 | | ||
| +-----------+---------+---------+-------+---------+----------+-------+ | ||
| +-----------+---------+---------+-------+---------+----------+----------+-------+ | ||
| | DatasetID | Version | Status | Rules | Success | Warnings | Failures | Skips | | ||
| +-----------+---------+---------+-------+---------+----------+----------+-------+ | ||
| | dataset1 | | SUCCESS | 49 | 43 | 4 | 0 | 2 | | ||
| +-----------+---------+---------+-------+---------+----------+----------+-------+ | ||
| ``` | ||
|
|
||
| ### Dump Results as JSON | ||
|
|
||
| To dump results into JSON, use the `-o; --output` option: | ||
|
|
||
| ```shell | ||
| t4sanity <DATA_ROOT> -o result.json | ||
| ``` | ||
|
|
||
| Then a JSON file named `result.json` will be generated as follows: | ||
|
|
||
| ```json | ||
| { | ||
| "dataset_id": "<DatasetID: str>", | ||
| "version": <Version: int>, | ||
| "reports": [ | ||
| { | ||
| "id": "<RuleID: str>", | ||
| "name": "<RuleName: str>", | ||
| "severity": "<ERROR/WARNING: str>", | ||
| "description": "<Description: str>", | ||
| "status": "<SUCCESS/FAILURE/SKIPPED: str>", | ||
| "status": "<SUCCESS/WARNING/FAILURE/SKIPPED: str>", | ||
| "reasons": "<[<Reason1>, <Reason2>, ...]: [str; N] | null>" // Failure or skipped reasons, null if success | ||
| }, |
There was a problem hiding this comment.
[imho]
Overall design discussion
IMHO, The current design looks more complex than it needs to be.
Right now both severity (ERROR / WARNING) and status (SUCCESS / WARNING / FAILURE / SKIPPED) overlap in meaning, which makes it hard to tell whether “WARNING” refers to rule importance or to the check’s outcome. This also makes options like --include-warning ambiguous — it’s unclear whether they skip execution, hide output, or influence exit codes.
To make the behavior clearer and closer to common linting/validation tools (flake8, pytest, pre-commit), I’d suggest the following adjustments:
-
Separate “severity” from “status”
• Severity → metadata of rule; defined per rule (WARNING, ERROR)
• Status → result of running the rule (PASSED, FAILED, SKIPPED)
• Example: a WARNING-severity rule can still fail, but that doesn’t necessarily cause the tool to exit with an error. -
Clarify CLI flag behavior
• --exclude → skip rule execution (status=SKIPPED)
• --show-warnings (rename from --include-warning) → control output visibility, not execution
• Optionally add --strict or --treat-warnings-as-errors for CI enforcement when warnings should fail the build -
Simplify exit status or API logic
Exit Status Logic
| Condition | --strict / --treat-warnings-as-errors |
Exit Code | Notes |
|---|---|---|---|
At least one ERROR-severity rule failed |
N/A | 1 | Always fails the run |
At least one WARNING-severity rule failed, no ERROR failures |
Off (default) | 0 | Run is considered successful; warnings are reported |
At least one WARNING-severity rule failed, no ERROR failures |
On | 1 | Treat warnings as errors; exit with failure |
| All rules passed or skipped | N/A | 0 | Success |
| Internal error / invalid CLI usage | N/A | >1 | Indicates tool or usage error |
- Adjust JSON and summary output
Example result schema:
{
"id": "STR001",
"severity": "ERROR",
"status": "FAILED",
"description": "something is not correct",
"reasons": [...]
}Summary table:
| Dataset | Version | Passed | Failed | Skipped | Warnings (count) |
|---|
Overall, this simplifies the mental model:
• Severity = how important a rule is
• Status = what happened when it ran
• Exit code/API call result = whether any error-level rules failed
That keeps the CLI behavior predictable, reduces confusion, and aligns well with established patterns for sanity/linting tools.
There was a problem hiding this comment.
@shekharhimanshu Thank you for detailed suggestions!
Separate “severity” from “status”
• Severity → metadata of rule; defined per rule (WARNING, ERROR)
• Status → result of running the rule (PASSED, FAILED, SKIPPED)
• Example: a WARNING-severity rule can still fail, but that doesn’t necessarily cause the tool to exit with an error.
I agree with you. In cebfaed, I addressed to remove Status.WARNING and return Status.SUCCESS for rules whose Severity == WARNING.
• Optionally add --strict or --treat-warnings-as-errors for CI enforcement when warnings should fail the build
I added -s; --strict option in 29f3e11.
This option enables us to treat rules whose Severity == WARNING as Status.FAILURE if they have warning reasons.
I also agree with your renaming suggestions, but let me address them in the another PR!
There was a problem hiding this comment.
Thank you!
I have one concern about this specification:
return Status.SUCCESS for rules whose Severity == WARNING.
since status is just the output of running a rule, status should be independent of severity. Therefore, if a WARNING severity rule fails, the report should have Status.FAILURE.
Severity comes into play to decide the overall result of validation.
- Default behavior (strict = false): Validation OK if there are no Status.FAILURE for severity=ERROR rules (Warning with Status.FAILURE are just reported in summary)
- strict = true behavior: Validation OK if there are no Status.FAILURE for both severity=ERROR & WARNING rules
This is my understanding of the rules design. Please check 🙏🏽
There was a problem hiding this comment.
since status is just the output of running a rule, status should be independent of severity. Therefore, if a WARNING severity rule fails, the report should have Status.FAILURE.
Severity comes into play to decide the overall result of validation.
- Default behavior (strict = false): Validation OK if there are no Status.FAILURE for severity=ERROR rules (Warning with Status.FAILURE are just reported in summary)
- strict = true behavior: Validation OK if there are no Status.FAILURE for both severity=ERROR & WARNING rules
Thank you, I updated in 3f9a1d3.
without `-s; --strict` option
$ t4sanity ./data/annotation_dataset/65785904-4f0a-4e3f-adf7-2935ecdb5db8/
=== DatasetID: 65785904-4f0a-4e3f-adf7-2935ecdb5db8 ===
FMT001: ✅
FMT002: ✅
FMT003: ✅
FMT004: ✅
FMT005: ✅
FMT006: ✅
FMT007: ✅
FMT008: ✅
FMT009: ✅
FMT010: ✅
FMT011: ✅
FMT012: ✅
FMT013: ✅
FMT014: ✅
FMT015: ✅
FMT016: ✅
FMT017: ✅
FMT018: ✅
REC001: ✅
REC002: ✅
REC003: ✅
REC004: ✅
REC005: ✅
REC006: ✅
REF001: ✅
REF002: ✅
REF003: ✅
REF004: ✅
REF005: ✅
REF006: ✅
REF007: ✅
REF008: ✅
REF009: ✅
REF010: ✅
REF011: ✅
REF012: [SKIPPED]
- Missing lidarseg.json
REF013: ✅
REF014: ✅
REF015: [SKIPPED]
- Missing data/annotation_dataset/65785904-4f0a-4e3f-adf7-2935ecdb5db8/annotation/lidarseg.json
STR001:
- 'version' directory doesn't exist
STR002: ✅
STR003: ✅
STR004:
- Path to 'map' not found: data/annotation_dataset/65785904-4f0a-4e3f-adf7-2935ecdb5db8/map
STR005: ✅
STR006: ✅
STR007: ✅
STR008:
- Path to 'map' directory not found: data/annotation_dataset/65785904-4f0a-4e3f-adf7-2935ecdb5db8/map
STR009:
- Path to 'map' directory not found: data/annotation_dataset/65785904-4f0a-4e3f-adf7-2935ecdb5db8/map
TIV001: ✅
+--------------------------------------+---------+---------+-------+---------+----------+-------+----------+
| DatasetID | Version | Status | Rules | Success | Failures | Skips | Warnings |
+--------------------------------------+---------+---------+-------+---------+----------+-------+----------+
| 65785904-4f0a-4e3f-adf7-2935ecdb5db8 | | SUCCESS | 49 | 49 | 0 | 2 | 4 |
+--------------------------------------+---------+---------+-------+---------+----------+-------+----------+with `-s; --strict` option
$ t4sanity ./data/annotation_dataset/65785904-4f0a-4e3f-adf7-2935ecdb5db8/ -s
=== DatasetID: 65785904-4f0a-4e3f-adf7-2935ecdb5db8 ===
FMT001: ✅
FMT002: ✅
FMT003: ✅
FMT004: ✅
FMT005: ✅
FMT006: ✅
FMT007: ✅
FMT008: ✅
FMT009: ✅
FMT010: ✅
FMT011: ✅
FMT012: ✅
FMT013: ✅
FMT014: ✅
FMT015: ✅
FMT016: ✅
FMT017: ✅
FMT018: ✅
REC001: ✅
REC002: ✅
REC003: ✅
REC004: ✅
REC005: ✅
REC006: ✅
REF001: ✅
REF002: ✅
REF003: ✅
REF004: ✅
REF005: ✅
REF006: ✅
REF007: ✅
REF008: ✅
REF009: ✅
REF010: ✅
REF011: ✅
REF012: [SKIPPED]
- Missing lidarseg.json
REF013: ✅
REF014: ✅
REF015: [SKIPPED]
- Missing data/annotation_dataset/65785904-4f0a-4e3f-adf7-2935ecdb5db8/annotation/lidarseg.json
STR001:
- 'version' directory doesn't exist
STR002: ✅
STR003: ✅
STR004:
- Path to 'map' not found: data/annotation_dataset/65785904-4f0a-4e3f-adf7-2935ecdb5db8/map
STR005: ✅
STR006: ✅
STR007: ✅
STR008:
- Path to 'map' directory not found: data/annotation_dataset/65785904-4f0a-4e3f-adf7-2935ecdb5db8/map
STR009:
- Path to 'map' directory not found: data/annotation_dataset/65785904-4f0a-4e3f-adf7-2935ecdb5db8/map
TIV001: ✅
+--------------------------------------+---------+---------+-------+---------+----------+-------+----------+
| DatasetID | Version | Status | Rules | Success | Failures | Skips | Warnings |
+--------------------------------------+---------+---------+-------+---------+----------+-------+----------+
| 65785904-4f0a-4e3f-adf7-2935ecdb5db8 | | FAILURE | 49 | 45 | 4 | 2 | 4 |
+--------------------------------------+---------+---------+-------+---------+----------+-------+----------+
|
@ktro2828 |
Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
ktro2828
force-pushed
the
feat/sanity/severity
branch
from
a7ed208 to
c8a6a3f
Compare
Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
ktro2828
force-pushed
the
feat/sanity/severity
branch
from
c8a6a3f to
29f3e11
Compare
…failed, but their reports are treated as sucuess unless strict=True Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
shekharhimanshu
left a comment
shekharhimanshu
left a comment
There was a problem hiding this comment.
LGTM. Thank you! 💯
Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
github-actions
Bot
added
the
ci
What
This PR is related to #212 (comment).
This pull request introduces a severity level system for sanity checkers, distinguishing between errors and warnings, and updates both the codebase and documentation to reflect these changes. It also updates the reporting format to include warnings and their counts, and revises some rules to be warnings instead of errors.
Key changes include:
Sanity Checker Severity and Reporting:
Severityenum (ERROR,WARNING) and updated theCheckerbase class to support severity levels for rules, affecting how results are reported and handled. (t4_devkit/sanity/checker.py, [1] [2]ERROR. (t4_devkit/sanity/format/fmt001.py, [1] [2]; ...and similar changes in allfmtXXX.pyfiles)ErrortoWarnto reflect their new severity. (docs/schema/requirement.md, docs/schema/requirement.mdL10-R12)Documentation and Output Format Updates:
severityfield and a newWARNINGstatus. (docs/cli/t4sanity.md, [1] [2]Codebase Consistency and Refactoring:
Severityenum and set the severity explicitly. (Allt4_devkit/sanity/format/fmtXXX.pyfiles, e.g., [1] [2] etc.)FieldTypeCheckerdocstring to document the newseverityattribute. (t4_devkit/sanity/format/base.py, t4_devkit/sanity/format/base.pyR24)Usage
There is no change about the usage of
t4sanityCLI andsanity_check(...)function, but the output JSON file contains a newseverityfield as folllows:{ "dataset_id": "65785904-4f0a-4e3f-adf7-2935ecdb5db8", "version": null, "reports": [ { "id": "FMT001", "name": "attribute-field", "severity": "ERROR", "description": "All types of 'Attribute' fields are valid.", "status": "SUCCESS", "reasons": null } ] }result.json