feat(shapes): emit JSON Schema if/then for Pose coco-17 arity (FILTER-454)#108
Conversation
lucasmundim
left a comment
There was a problem hiding this comment.
No bugs found — the if/then is a faithful mirror of _validate_skeleton_arity (the required: ["skeleton"] correctly keeps the then from firing on absent/null skeletons), it coexists cleanly with the $id merge in __init_subclass__, and it rides into $defs[Pose] as the tests assert. Two non-blocking suggestions below.
shingonoide
left a comment
There was a problem hiding this comment.
Reviewed at 408f342. This is a clean Slice A implementation of FILTER-454 and the scoping is right: the coco-17 arity is the one invariant expressible in stock JSON Schema, and deferring the x-openfilter-* vocabulary (Slice B) until PLAT-1012 lands matches the ticket.
I confirmed the emitted schema against a stock Draft 2020-12 validator (jsonschema):
- 17 keypoints + skeleton="coco-17" -> valid
- 16 keypoints + skeleton="coco-17" -> rejected
- no-skeleton pose -> unconstrained
So the if/then mirrors _validate_skeleton_arity exactly for raw-JSON consumers. The if trigger (required: ["skeleton"] plus const: "coco-17") fires only on the coco-17 discriminator, and minItems/maxItems are the correct keywords for the array-length constraint. The $id merge through init_subclass holds: both if/then and $id survive into the top-level schema and into $defs[Pose] when referenced from PoseSet, which the new tests pin.
One thing before this can go green: the check-release-log job is failing with "RELEASE.md must be updated on every PR to main." Please add a RELEASE.md entry for this change. Everything else (unit tests 3.10 through 3.13, grype, DCO) is passing.
Minor, optional: test_pose_arity_if_then_travels_into_defs only asserts if/then are present in $defs[Pose], not their values. Asserting the actual minItems/maxItems there too would catch a future regression that emits an empty or wrong constraint into $defs. Nice, minimal change otherwise.
d475245 to
61422d2
Compare
shingonoide
left a comment
There was a problem hiding this comment.
Slice A looks correct at 61422d2: SKELETON_ARITY is a single source of truth, the if/then is semantically right, and Pose.emit_schema() validates as intended against a stock Draft 2020-12 validator (17 keypoints valid, 16 and 18 rejected, absent skeleton unconstrained). All prior asks are addressed.
One observation worth a follow-up (non-blocking, and the root cause predates this PR): the arity is enforced only when Pose.emit_schema() is the root schema. When Pose is embedded via $defs in a parent schema (for example PoseSet), a stock Draft 2020-12 validator silently does not apply the if/then, so a 16- or 18-keypoint coco-17 pose inside a PoseSet passes validation. The cause is the $id on the $defs.Pose entry, which rebases the embedded subschema during $ref resolution. Since pose data is generally emitted as a set, this limits how much the if/then protects consumers end to end. A FILTER-444 follow-up to avoid $id on embedded $defs (or to inline the conditional) would close the gap.
Minor: the if/then is built with next(iter(SKELETON_ARITY)), so it encodes only the first dict entry. If a second skeleton is ever added, the validator would enforce all entries but the emitted if/then would only cover the first, reintroducing the asymmetry this PR closes. A small allOf of per-skeleton if/then pairs would keep it robust.
lucasmundim
left a comment
There was a problem hiding this comment.
Approving — slice A is correct and well-scoped at 61422d2.
Both of my prior comments are addressed cleanly:
SKELETON_ARITY = {"coco-17": 17}is now the single source of truth driving both_validate_skeleton_arityand the emittedif/then.test_pose_arity_if_then_travels_into_defsnow asserts fullif/thenequality againstPose.emit_schema(), not just key presence.
I re-validated Pose.emit_schema() against a stock Draft 2020-12 validator (jsonschema): a 17-keypoint coco-17 pose validates, 16 is rejected, and an absent skeleton is unconstrained — so the if/then mirrors _validate_skeleton_arity exactly for raw-JSON consumers. RELEASE.md entry is in, and the test suite is green.
Non-blocking notes (both fine to defer):
- The
uv.lockchange just drops a stalerequires-devblock that no longer has apyproject.tomlsource (left over from #86);uv lock --checkpasses, so the lock is back in sync. - The embedded-
$defslimitation shingonoide raised is real and worth the FILTER-444 follow-up. Worth noting it's actually a hard failure, not just a silent skip: the$idon$defs.Poserebases the subschema, so the innerkeypoints$ref: "#/$defs/Keypoint"becomes a dangling pointer (PointerToNowhere) and a stock validator throws on any embedded PoseSet. Pre-existing and out of scope for slice A.
…-454) ## 📋 What does this PR do? Makes the `Pose` output shape emit standard JSON Schema (draft 2020-12) `if`/`then` for the coco-17 keypoint-arity invariant: when `skeleton == "coco-17"`, `keypoints` must contain exactly 17 entries (`minItems`/`maxItems`: 17). The constraint is injected via `Pose.model_config.json_schema_extra`, so it flows through `model_json_schema()` → `emit_schema()` with no change to the base `FilterOutputSchema.emit_schema()`. The base `__init_subclass__` merges `$id` into the same `json_schema_extra` dict, so identity and constraint co-exist, and the `if`/`then` rides into `$defs[Pose]` wherever Pose is `$ref`'d (e.g. `PoseSet`). ## 🔍 Why is this needed? `Pose._validate_skeleton_arity` enforces the coco-17 arity for Pydantic producers, but consumers validating raw JSON against `emit_schema()` — Go (santhosh-tekuri), TS (Ajv) — get nothing, so they silently accept malformed poses. This is the producer/consumer asymmetry FILTER-454 exists to close. The coco-17 case is the one invariant expressible in *stock* JSON Schema, so it lands now as "slice A" with zero custom-keyword infrastructure. The two genuinely-custom keywords (`x-openfilter-xyxy-ordering`, `x-openfilter-parallel-arrays`) are slice B, blocked on the public validator-home decision. ## 🧪 How was it tested? - `pytest tests/test_output_schema.py tests/test_emit_schema_cli.py` — 54 passed. - New tests: `test_pose_emits_coco17_arity_if_then` (asserts the `if`/`then` at the schema top level with `$id` intact) and `test_pose_arity_if_then_travels_into_defs` (asserts it rides into `$defs[Pose]` when referenced from `PoseSet`). - `ruff check` clean on the changed files; `black` clean on the additions. ## 🔗 Related Issues Implements slice A of [FILTER-454](https://plainsight-ai.atlassian.net/browse/FILTER-454). ## ✅ Checklist - [x] I have read and agreed to the terms of the LICENSE - [x] I have read the CONTRIBUTING guide - [x] I have followed the coding style (ruff clean; additions black-clean; no unrelated reformatting — repo has no black/ruff-format CI or line-length override) - [x] I have signed all commits in compliance with the DCO (`git commit -s`) - [x] I have added or updated tests as needed - [x] Documentation: the `Pose` docstring already states the enforced coco-17 arity; vocabulary-level docs are slice B Signed-off-by: stwilt <swilt@plainsight.ai>
Signed-off-by: stwilt <swilt@plainsight.ai>
61422d2 to
1eb3f07
Compare
📋 What does this PR do?
Makes the
Poseoutput shape emit standard JSON Schema (draft 2020-12)if/thenfor the coco-17 keypoint-arity invariant: whenskeleton == "coco-17",keypointsmust contain exactly 17 entries (minItems/maxItems: 17). The constraint is injected viaPose.model_config.json_schema_extra, so it flows throughmodel_json_schema()→emit_schema()with no change to the baseFilterOutputSchema.emit_schema(). The base__init_subclass__merges$idinto the samejson_schema_extradict, so identity and constraint co-exist, and theif/thenrides into$defs[Pose]wherever Pose is$ref'd (e.g.PoseSet).🔍 Why is this needed?
Pose._validate_skeleton_arityenforces the coco-17 arity for Pydantic producers, but consumers validating raw JSON againstemit_schema()— Go (santhosh-tekuri), TS (Ajv) — get nothing, so they silently accept malformed poses. This is the producer/consumer asymmetry FILTER-454 exists to close. The coco-17 case is the one invariant expressible in stock JSON Schema, so it lands now as "slice A" with zero custom-keyword infrastructure. The two genuinely-custom keywords(
x-openfilter-xyxy-ordering,x-openfilter-parallel-arrays) are slice B, blocked on the public validator-home decision.🧪 How was it tested?
pytest tests/test_output_schema.py tests/test_emit_schema_cli.py— 54 passed.test_pose_emits_coco17_arity_if_then(asserts theif/thenat the schema top level with$idintact) andtest_pose_arity_if_then_travels_into_defs(asserts it rides into$defs[Pose]when referenced fromPoseSet).ruff checkclean on the changed files;blackclean on the additions.🔗 Related Issues
Implements slice A of FILTER-454.
✅ Checklist
git commit -s)Posedocstring already states the enforced coco-17 arity; vocabulary-level docs are slice BThanks for contributing to OpenFilter! We’ll review your PR as soon as possible.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.