Skip to content

fix(sanity): check REC006 only if either SampleAnnotation or ObjectAnn is not empty#239

Merged
ktro2828 merged 3 commits into
mainfrom
fix/sanity/remove-rec006
Dec 3, 2025
Merged

fix(sanity): check REC006 only if either SampleAnnotation or ObjectAnn is not empty#239
ktro2828 merged 3 commits into
mainfrom
fix/sanity/remove-rec006

Conversation

@ktro2828

@ktro2828 ktro2828 commented Dec 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator

What

This pull request updates the logic and documentation for the REC006 schema requirement to ensure the Instance record is only required to be non-empty if either SampleAnnotation or ObjectAnn records exist and are non-empty. The most important changes are grouped below by documentation and implementation updates.

Documentation Update:

  • Clarified the description of REC006 in docs/schema/requirement.md to specify that the Instance record must be non-empty only if either SampleAnnotation or ObjectAnn is not empty.

Implementation Updates:

  • Updated the REC006 checker class in t4_devkit/sanity/record/rec006.py to reflect the new requirement, including an improved description and logic for skipping the check when appropriate.
  • Added a can_skip method to REC006 that checks for the existence and content of SampleAnnotation and ObjectAnn files, allowing the check to be skipped if both are missing or empty, or if annotation files are missing.
  • Imported necessary modules (Maybe, Nothing, Some, and load_json_safe) and added type checking imports to support the new logic in the checker.

Related Links

How PR Tested?

I confirmed this PR passed sanity checks using the above sample dataset:

  • Before
  REC006:
     - 'Instance' record must not be empty
+--------------------------------------+---------+--------+--------+---------+----------+
|              DatasetID               | Version | Passed | Failed | Skipped | Warnings |
+--------------------------------------+---------+--------+--------+---------+----------+
| 40b478fd-6e8b-499c-b6e1-ae94744279fb |         |   48   |   1    |    2    |    4     |
+--------------------------------------+---------+--------+--------+---------+----------+
  • After
  REC006: [SKIPPED]
     - Both sample_annotation and object_ann records are empty
+--------------------------------------+---------+--------+--------+---------+----------+
|              DatasetID               | Version | Passed | Failed | Skipped | Warnings |
+--------------------------------------+---------+--------+--------+---------+----------+
| 40b478fd-6e8b-499c-b6e1-ae94744279fb |         |   49   |   0    |    3    |    4     |
+--------------------------------------+---------+--------+--------+---------+----------+

@ktro2828
fix: remove REC006
a9b763e 
Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
Copilot AI review requested due to automatic review settings December 2, 2025 10:11
@github-actions github-actions Bot added bug Something isn't working ci Continuous Integration (CI) processes and testing documentation Improvements or additions to documentation labels Dec 2, 2025
Copilot AI reviewed Dec 2, 2025
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This pull request removes the REC006 sanity checker, which previously enforced that the Instance record in T4 datasets must not be empty. The change accommodates datasets that legitimately contain no sample annotations, such as certain T4 dataset cases where instance.json may be an empty array.

Key Changes:

  • Removed the REC006 checker implementation and all references to it
  • Deleted associated test cases for the removed checker
  • Updated documentation to reflect the removal of this requirement

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
t4_devkit/sanity/record/rec006.py Deleted the entire REC006 checker implementation file
t4_devkit/sanity/record/init.py Removed import of the deleted REC006 module
docs/schema/requirement.md Removed REC006 entry from the requirements table
tests/sanity/test_record_checkers.py Removed import statement and test functions for REC006

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions

github-actions Bot commented Dec 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
3987 3281 82% 50% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
t4_devkit/sanity/record/rec006.py 94% 🟢
TOTAL 94% 🟢

updated for commit: 6369902 by action🐍

@ktro2828 ktro2828 requested a review from Copilot December 2, 2025 10:13
Copilot AI reviewed Dec 2, 2025
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

shekharhimanshu
shekharhimanshu approved these changes Dec 2, 2025
View reviewed changes

@shekharhimanshu shekharhimanshu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Thank you! 💯

@ktro2828 ktro2828 changed the title fix: remove REC006 fix(sanity): remove REC006 Dec 2, 2025
@shekharhimanshu

shekharhimanshu commented Dec 3, 2025

Copy link
Copy Markdown
Contributor

@ktro2828

Just one confirmation.

In T4 dataset, there is a case that the dataset doesn't contain any sample_annotation.
In this case, sanity checking should allow even if instance.json is empty [].

It looks like instance.json existence is conditional (based on existence of sample_annotation). Does that mean, instead of removing this rule altogether, is it better to have a conditional check or convert this to warning?

@ktro2828

ktro2828 commented Dec 3, 2025

Copy link
Copy Markdown
Collaborator Author

@shekharhimanshu Thank you for your comments.

The existence of records of instance.json varies depending on the following conditions:

Condition Existence
SampleAnnotation or ObjectAnn exists Yes
Otherwise No

So, as you suggested, it would better for us to keep REC006.
And we should run checking if either of them exists, otherwise skip checking.

@ktro2828
Revert "fix: remove REC006"
0e374ce 
This reverts commit a9b763e.

Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
@ktro2828 ktro2828 changed the title fix(sanity): remove REC006 fix(sanity): check REC006 only if either SampleAnnotation or ObjectAnn is not empty Dec 3, 2025
@ktro2828

ktro2828 commented Dec 3, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

In 6369902, I updated to check REC006 only if either 'SampleAnnotation' or 'ObjectAnn' is not empty.
The tested result is as below:

  REC006: [SKIPPED]
     - Both sample_annotation and object_ann records are empty
+--------------------------------------+---------+--------+--------+---------+----------+
|              DatasetID               | Version | Passed | Failed | Skipped | Warnings |
+--------------------------------------+---------+--------+--------+---------+----------+
| 40b478fd-6e8b-499c-b6e1-ae94744279fb |         |   49   |   0    |    3    |    4     |
+--------------------------------------+---------+--------+--------+---------+----------+

@shekharhimanshu

shekharhimanshu commented Dec 3, 2025

Copy link
Copy Markdown
Contributor

Thanks for #239 (comment).

can I confirm if the condition is OR or AND for sample_annotation & object_ann records?

SampleAnnotation or ObjectAnn exists
Both sample_annotation and object_ann records are empty

Will REC006 be skipped if either one of them is empty for example?

@ktro2828
fix: update to run checking only if either sample_ann or object_ann i…
6369902 
…s not empty

Signed-off-by: ktro2828 <kotaro.uetake@tier4.jp>
@ktro2828 ktro2828 force-pushed the fix/sanity/remove-rec006 branch from 3ad442d to 6369902 Compare December 3, 2025 02:03
@ktro2828

ktro2828 commented Dec 3, 2025

Copy link
Copy Markdown
Collaborator Author

@shekharhimanshu

can I confirm if the condition is OR or AND for sample_annotation & object_ann records?

We take OR in this case. So, if either one of sample_annotation or object_ann is not empty, instance must contains some records.

Will REC006 be skipped if either one of them is empty for example?

No, it will be skipped only if both of them are empty.

SampleAnnotation ObjectAnn Check REC006? (Expect instance is not empty)
✅ ✅ Yes
✅ ❌ Yes
❌ ✅ Yes
❌ ❌ No (Skip)

@shekharhimanshu

shekharhimanshu commented Dec 3, 2025

Copy link
Copy Markdown
Contributor

Sounds good. Thanks for the changes and clarifications! LGTM 👍🏽

@ktro2828 ktro2828 merged commit 0533e6d into main Dec 3, 2025
4 of 5 checks passed
@ktro2828 ktro2828 deleted the fix/sanity/remove-rec006 branch December 3, 2025 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@shekharhimanshu shekharhimanshu shekharhimanshu approved these changes

@SamratThapa120 SamratThapa120 Awaiting requested review from SamratThapa120

Assignees

No one assigned

Labels

bug Something isn't working ci Continuous Integration (CI) processes and testing documentation Improvements or additions to documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ktro2828 @shekharhimanshu