Use quartz_tracking_id for owner run-id and release_type classification - #87
Conversation
marbre
left a comment
marbre
left a comment
There was a problem hiding this comment.
Two findings from my agent. Posted the first at the function, whereas the second might not be valid and depends on how this is handled cross-repo. Might need a fallback for JAX or might not, but please check before landing:
Finding 2: Removing the artifact_run_id/URL-based owner fallback may regress owner resolution for external PyTorch/JAX release triggers
File: scripts/receive_therock/therock_classify.py:311 (derive_effective_owner_run_id)
The PR removes the artifact_run_id/URL-based owner fallback in favor of reading the owner solely from the propagated quartz_tracking_id. The deleted test test_falls_back_to_find_links_url_run_id documented that:
Upstream benc-uk dispatches that don't carry an
artifact_run_idinput
(e.g. the real PyTorch/JAX release triggers) still carry the top-level run
id baked into the artifact bucket path.
If TheRock cannot add a new quartz_tracking_id input to those externally-hosted workflows — the same constraint that historically prevented adding artifact_run_id or rocm_version there, per therock_types_test.py's own comment on test_pytorch_wheels_full.yml — those runs would now resolve to no owner at all.
The existing _is_ownerless_pytorch_leaf_for_release escape hatch (therock_update_status_json.py:898) only covers pipeline_type == "pytorch" (explicitly "the sole ownerless-acceptance path", line 906-907 comment). An equivalent externally-dispatched JAX release-trigger run has no fallback and would be silently excluded from status.json.
Caveat: this can't be fully confirmed from the Quartz repo alone — it depends on whether TheRock's paired change actually propagates the new quartz_tracking_id input to those specific external workflows. Worth confirming with the TheRock-side companion PR before/at merge.
| def parse_quartz_tracking_id(inputs: dict[str, Any]) -> tuple[int | None, str | None]: | ||
| """Split the propagated `quartz_tracking_id` into (owner_run_id, release_type). | ||
|
|
||
| The top-level `multi_arch_release.yml` orchestrator stamps every workflow it | ||
| triggers with `quartz_tracking_id: "<github.run_id>;<release_type>"` (empty | ||
| when tracking is disabled). `github.run_id` is the orchestrator's own run, the | ||
| top-level owner of the whole release lineage, and `release_type` is the | ||
| channel it published to. Both are authoritative for every descendant run, so | ||
| they are read straight from here rather than reconstructed from artifact ids, | ||
| URLs, or the immediate GitHub parent. | ||
|
|
||
| Returns `(None, None)` when the input is absent or empty (CI runs, manual | ||
| TheRock dispatches, and the orchestrator's own record, which generates the id | ||
| but does not carry it on its own inputs). | ||
| """ | ||
| raw = inputs.get("quartz_tracking_id") | ||
| if not isinstance(raw, str) or not raw.strip(): | ||
| return None, None | ||
| run_id_part, _, release_type_part = raw.partition(";") | ||
| return _parse_int(run_id_part.strip()), release_type_part.strip() or None |
There was a problem hiding this comment.
My review agent was flagging the following with regards to this function:
Malformed quartz_tracking_id is silently coerced to None instead of raising
File: scripts/receive_therock/therock_types.py:72 (the _parse_int helper it calls is at line 44; parse_quartz_tracking_id def at line 53)
def _parse_int(value: Any) -> int | None:
if value in (None, ""):
return None
try:
return int(str(value))
except ValueError:
return None
def parse_quartz_tracking_id(inputs: dict[str, Any]) -> tuple[int | None, str | None]:
...
raw = inputs.get("quartz_tracking_id")
if not isinstance(raw, str) or not raw.strip():
return None, None
run_id_part, _, release_type_part = raw.partition(";")
return _parse_int(run_id_part.strip()), release_type_part.strip() or None # line 72_parse_int swallows ValueError and returns None for any non-numeric string. This violates the project's fail-fast convention: a malformed quartz_tracking_id (e.g. "123abc;nightly" instead of "123;nightly", producer-side bug) should be a loud failure, not a silent None.
Confirmed directly:
>>> parse_quartz_tracking_id({"quartz_tracking_id": "123abc;nightly"})
(None, "nightly")Why it isn't caught upstream
release_type is parsed independently from the second half of the same string, in WorkflowRunRecord.from_dict (therock_types.py:761, parsing at lines 795-796). So release_typestill resolves to"nightly"` correctly even when the run-id half is garbage — the run looks "tracked" and nothing about it looks broken on the surface.
Where it actually bites
derive_effective_owner_run_id(scripts/receive_therock/therock_classify.py:311) callsparse_quartz_tracking_idagain at line 327 and returns theNonerun-id aswr.trigger_workflow_run_id.- That flows into
_gate_to_document_owner(scripts/receive_therock/therock_update_status_json.py:915):owner = doc.trigger_workflow_run_id(line 926) — a real run id from the existing status.json document, notNone/0.parent = workflow_run.trigger_workflow_run_id(line 927) —None, because of the bug.- Falls into the
parent in (None, 0)branch (line 939):
workflow_run.workflow_run_id == owneris false (this is a leaf run, not the top-level orchestrator), and_is_ownerless_pytorch_leaf_for_release(line 898, restricted topipeline_type == "pytorch"at line 909) doesn't rescue a non-pytorch leaf. - Result:
_gate_to_document_ownerreturnsFalse; the update is skipped with only anlog.infoat lines 947-952 — no warning, no error, no exception.
Net effect
A malformed quartz_tracking_id from any producer-side bug causes a real leaf run's data to be silently dropped from status.json, logged at info level indistinguishably from the legitimate "this run doesn't belong to this release" case. No existing test in therock_types_test.py or therock_classify_test.py exercises a non-numeric run-id half.
Suggested fix direction
parse_quartz_tracking_id should raise (e.g. ValueError) when the run-id half is present but non-numeric, distinguishing that from the legitimate "absent tracking id" case (None, None) — so a corrupted producer payload fails loudly instead of being silently absorbed into the ownerless-leaf gating path.
There was a problem hiding this comment.
addressed
|
discarding: "Finding 2: Removing the artifact_run_id/URL-based owner fallback may regress owner resolution for external PyTorch/JAX release triggers" |
![quartz-sync[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
3 participants
Summary
TheRock now propagates a
quartz_tracking_idinput on every workflow ittriggers, formatted as
<github.run_id>;<release_type>(empty when tracking isdisabled). The
github.run_idhalf is the top-levelmulti_arch_release.ymlorchestrator run that owns the whole release lineage, and the
release_typehalf is the channel it published to. Both values are authoritative, so we read
them straight from this field instead of reconstructing them.
This replaces the previous heuristic chains for both derivations. The owner
run-id no longer walks
artifact_run_idinputs, artifact-bucket URL parsing, orthe immediate GitHub parent; the release_type no longer falls back to a
top-level
release_typekey or theRELEASE_TYPEenv var. The orchestrator'sown record still owns itself via the existing base case (it generates the id but
does not carry it on its own inputs), and
inputs.release_typeis retained asthe direct-input fallback for that record and for manual dispatches.
Changes
parse_quartz_tracking_id()intherock_types.pyas the single splitpoint for the
<run_id>;<release_type>field, shared byfrom_dictand theclassifier.
derive_effective_owner_run_id()reads the owner fromquartz_tracking_id;delete the
artifact_run_id/ URL-parsing / parent-walk reconstruction(
_input_int,_input_url_run_id,_ARTIFACT_URL_RUN_ID_RE).from_dictsourcesrelease_typefromquartz_tracking_idfirst, then theinputs.release_typefallback.KNOWN_RELEASE_TYPESmirroring the orchestrator's own enum, andextend the accepted set to include the
dev-bkcandnightly-bkcchannels(previously coerced to None). This is the input accept-list and is
deliberately broader than the
_TRACKED_RELEASE_TYPESpublish subset.derive_source_run_id(artifact-bucket run id) is unchanged; it is aseparate concept and still reads the immediate GitHub parent before the owner
is normalized.
Test plan
pytest tests/therock_classify_test.py tests/therock_types_test.py;<release_type>suffix; missing/empty id returns None; orchestrator ownsitself.
inputs.release_type; input fallback stillworks;
dev-bkc/nightly-bkcpass through; unrecognized coerced to None.classify()ordering:source_run_idstill keys off the immediate parent.