Skip to content

Resync the vendored omni-github result schema with upstream - #6897

Open
Neil4561 wants to merge 2 commits into
isaac-sim:developfrom
NVIDIA-Omniverse:neilm/omni-github-schema-resync
Open

Resync the vendored omni-github result schema with upstream#6897
Neil4561 wants to merge 2 commits into
isaac-sim:developfrom
NVIDIA-Omniverse:neilm/omni-github-schema-resync

Conversation

@Neil4561

@Neil4561 Neil4561 commented Aug 4, 2026

Copy link
Copy Markdown

Follows up on @fatima-anes's review of #6433: rather than patching one rule of a stale copy, this refreshes the whole vendored schema from upstream.

Why

.github/actions/upload-omni-github-test-results/result-json.schema.json is a pinned copy of the schema in NVIDIA-Omniverse/omni-github at docs/test-results/clients/artifact-upload/schemas/result-json.schema.json. Pinning is deliberate — action.yml notes the omni-github team recommends validating against a local copy — but ours was taken once in #6234 on 2026-06-26 and never refreshed.

That matters because the upload action treats this file as a hard gate and silently drops any result that fails validation:

Skipping omni-github upload because converted results failed schema validation

A stale copy therefore rejects payloads the service itself would accept, and says nothing about it.

What changed

The file is now byte-identical to upstream. The drift is entirely additive — upstream gained $defs.profileDetailString and seven optional testRow fields:

error, failure_component, failure_layer, reliability_class, retry_kind, retry_reason, test_category

Nothing this repository relies on was removed upstream, and no producer here emits the new fields yet, so this is a no-op for current uploads and unblocks the new ones.

Relationship to #6433

Upstream already uses anyOf for customValue, so #6433 was correcting our copy toward upstream rather than diverging from it. This branch is based on #6433 and subsumes it.

Either order works. If #6433 merges first this reduces to the remaining drift; if it doesn't, #6433 can be closed as superseded. #6433 is the smaller, already-approved change, so merging it first is the lower-risk path — it is what unblocks the perf-smoke gate's dashboard rows.

Verification

  • The vendored file is byte-identical to upstream (cmp).
  • test_junit_to_omni_github_results.py passes (4 tests).
  • The converter's real output validates against the resynced schema.
  • The perf-smoke gate's result JSON validates against it too, unrelaxed — worth stating because that gate emits integral custom values (num_envs: 4096, fps_mean), which is precisely what Fix omni-github result schema rejecting integral numeric custom values #6433 addresses.

Follow-up, not in this PR

The perf-smoke gate carries a _relax_custom_value_oneof test helper that rewrites oneOf to anyOf in memory before validating, added while #6433 was pending. It should be deleted once this or #6433 lands, but it lives in tools/perf_smoke_test/, which is not on develop yet, so it belongs with #6864.

Also worth considering separately: failure_layer (code | infrastructure | environment | upstream | downstream) and retry_kind map closely onto distinctions the perf gate already makes — a stale CI image is an environment failure, not a code one. Today that only exists as prose in the PR comment.

Neil4561 and others added 2 commits July 9, 2026 00:56
$defs.customValue (and its nested array items) used oneOf listing both
integer and number. Under JSON Schema draft 2020-12 integer is a subset
of number, so any integral value (e.g. 4096 or 100.0) matches both
branches and oneOf rejects it. The upload action enforces this schema as
a gate and skips the upload on validation failure, so a producer emitting
integral numeric custom values has its results silently dropped.

Change both oneOf to anyOf to preserve the intended union while allowing
integral numbers.
The copy under .github/actions/upload-omni-github-test-results was taken
once in isaac-sim#6234 and never refreshed, so it has drifted from the source in
NVIDIA-Omniverse/omni-github at docs/test-results/clients/artifact-upload/
schemas/result-json.schema.json. The upload action validates every result
file against this copy and silently drops anything that fails, so a stale
copy can reject payloads the service itself would accept.

Replace it wholesale rather than patching individual rules. The file is
now byte-identical to upstream.

The drift is additive: upstream gained profileDetailString and seven
optional testRow fields (error, failure_component, failure_layer,
reliability_class, retry_kind, retry_reason, test_category). Nothing this
repository relies on was removed. The converter's own output and the
perf-smoke gate's result JSON both validate against the resynced schema
unchanged.

This also subsumes the oneOf -> anyOf fix in isaac-sim#6433, which upstream had
already made; that PR remains the minimal standalone fix if this lands
after it.
@Neil4561
Neil4561 requested a review from a team August 4, 2026 20:59
@greptile-apps

greptile-apps Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Resynchronizes the vendored omni-github result schema with upstream, expanding accepted test metadata and aligning custom-value validation.

  • Adds seven optional test-row metadata fields and their shared string definition.
  • Changes customValue overlap handling from oneOf to anyOf.
  • Updates array-valued custom fields to use upstream's homogeneous item schemas.

Confidence Score: 5/5

The PR appears safe to merge, with no concrete repository payload path found that regresses under the synchronized schema.

The local result converter does not emit the newly added metadata or array-valued custom fields, while the schema changes expand accepted metadata and restore upstream custom-value semantics.

Important Files Changed

Filename Overview
.github/actions/upload-omni-github-test-results/result-json.schema.json Aligns the vendored schema with upstream through additive metadata fields and updated custom-value validation, with no actionable regression identified for repository-produced payloads.

Reviews (1): Last reviewed commit: "Resync the vendored omni-github result s..." | Re-trigger Greptile

@isaaclab-review-bot isaaclab-review-bot Bot 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.

Isaac Lab Review Bot

Reviewed the vendored omni-github schema resync, including the added optional testRow fields, the new shared string definition, and the customValue validation changes. No actionable finding is supported by the supplied patch.

  • Design and architecture: Keeping the repository’s local validation schema synchronized with the upstream service schema is consistent with the pinned-vendor design. The patch does not alter workflow wiring or producer paths.
  • API: Existing required testRow fields remain unchanged, while the seven new fields are optional and valid under additionalProperties: false. The new $ref targets resolve correctly. The array-level anyOf makes custom arrays homogeneous, so this refresh is not strictly additive for previously accepted mixed-type arrays, but that behavior matches the stated upstream contract and does not establish a defect in this vendor resync.
  • Implementation: The added definitions, references, enums, and type constraints are internally consistent. Changing scalar customValue from oneOf to anyOf permits integers that also satisfy JSON Schema’s number type. The supplied verification covers the repository converter output and perf-smoke payload; no changed producer emits the newly added fields.

No blocking issues. No inline issue met the actionable-evidence threshold; the assessment above records the review feedback.

Automated review; human maintainers own approval decisions.

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.

1 participant