Skip to content

fix(memory): restore RegisteredMemoryContext::device, broken on main - #1762

Merged
justinchuby merged 1 commit into
mainfrom
squad/restore-context-device
Aug 22, 2026
Merged

justinchuby merged 1 commit into
mainfrom
squad/restore-context-device

Conversation

@justinchuby

Copy link
Copy Markdown
Owner

main does not compile in the CUDA lane, and my own #1725 is what broke it. Flagging that first because it gates every PR currently open.

error[E0599]: no method named `device` found for struct `RegisteredMemoryContext`
  --> crates/onnx-runtime-ep-cuda/src/provider.rs:1163:30
error: could not compile `onnx-runtime-ep-cuda` (lib)
exit=101

First red at a2cd1e7bf; a2a61656f immediately before it is green. git show ab0880b69 (#1725) contains the removal.

What happened

#1725 rewrote the use list in manager.rs to pull in the pin-source types, and pub fn device was deleted along with it. Nothing in that PR required its removal — the underlying RegisteredProviderContext::device is untouched — so this restores the forwarder verbatim.

Why nothing caught it, which is the part worth keeping

Three independent things had to line up, and each is individually reasonable:

  1. onnx-runtime-ep-cuda has exactly one caller of this method, and it compiles in exactly one job. Every lane that ran on feat(memory): pin the provider/context across a plugin's deferred release #1725 was blind to it by construction.
  2. I verified feat(memory): pin the provider/context across a plugin's deferred release #1725 with cargo clippy --all-targets -- -D warnings on the three crates I had edited. That is a real check, and it is exactly the crate set that cannot observe this break. Editing crate A and verifying crate A is the natural thing to do and it is not sufficient when A is a library.
  3. A pub method with no in-crate caller draws no dead_code warning. So deleting it was silent going out, and the breakage was silent coming back — until a job that most PRs never reach.

This is the same shape as the kv_commits_on_demand finding in #1631/#1632: several individually-sound reasons not to look, converging. I had that lesson in hand and still shipped this, so the lesson evidently was not "run clippy" — it was verify the reverse dependencies of a library change, not the library.

Verification — the lane's own command, not a proxy

cargo check -p onnx-runtime-ep-cuda -p onnx-runtime-python --features onnx-runtime-python/cuda

main  -> exit 101 (the E0599 above)
fixed -> exit 0

Negative control taken deliberately: I stashed the fix and re-ran on clean main to confirm the error returns, so this is demonstrably the cause rather than a change that merely happens to compile.

Both results reproduced locally on macOS aarch64 with no CUDA toolkit and no GPU. That matters — this was catchable on any developer machine, which means it was not a CUDA-hardware coverage gap.

Also clean: cargo fmt --all -- --check; clippy -D warnings on the governor; 67 + 34 + 5 governor tests; and the native-CUDA clippy lane from #1632.

Not fixed here

Mobius metadata packages is also red, with The process '/usr/bin/git' failed with exit code 1 and no compile error. Different failure, unrelated to this change, and I have not diagnosed it.

`main` does not compile in the CUDA lane. My own #1725 (ab0880b) did it,
and I am the one who missed it.

    error[E0599]: no method named `device` found for struct
                  `RegisteredMemoryContext`
      --> crates/onnx-runtime-ep-cuda/src/provider.rs:1163:30

The deletion was collateral. #1725 rewrote the `use` list in manager.rs to
bring in the pin-source types, and `pub fn device` went out with it. Nothing
in that PR needed it gone; the underlying `RegisteredProviderContext::device`
is untouched, so this restores the forwarder verbatim.

Why review and CI both passed it: `onnx-runtime-ep-cuda` is the only caller,
and it compiles in exactly one job. Every lane that ran on #1725 was blind to
it by construction, and I verified #1725 with `cargo clippy` on the three
crates I had edited -- which is precisely the crate set that cannot observe
this break. A pub method with no in-crate caller also draws no dead_code
warning, so deleting it was silent in both directions.

Verified with the lane's own command rather than a proxy:

    cargo check -p onnx-runtime-ep-cuda -p onnx-runtime-python \
      --features onnx-runtime-python/cuda

    main  -> exit 101, the E0599 above
    fixed -> exit 0

Both reproduced locally on macOS aarch64 with no CUDA toolkit and no GPU,
which is worth noting: this was catchable on any developer machine.

Also clean: fmt; clippy -D warnings on the governor; 106 governor tests; and
the native-cuda clippy lane from #1632.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c80f8522-983c-47f7-8241-2155a823aabe
@justinchuby
justinchuby merged commit 233904b into main Aug 22, 2026
13 of 15 checks passed
@justinchuby
justinchuby deleted the squad/restore-context-device branch August 22, 2026 17:33
@codecov

codecov Bot commented Aug 22, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.11%. Comparing base (6fdc04d) to head (a65fc5d).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
crates/onnx-runtime-memory-governor/src/manager.rs 0.00% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1762      +/-   ##
==========================================
- Coverage   80.11%   80.11%   -0.01%     
==========================================
  Files         411      411              
  Lines      198646   198649       +3     
  Branches   198646   198649       +3     
==========================================
- Hits       159151   159146       -5     
- Misses      34094    34101       +7     
- Partials     5401     5402       +1     
Flag Coverage Δ
cli-ort-linux 72.47% <ø> (ø)
cli-ort-windows 71.97% <ø> (ø)
mlas 85.10% <ø> (-0.10%) ⬇️
offline 80.24% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
crates/onnx-runtime-memory-governor/src/manager.rs 75.68% <0.00%> (-0.10%) ⬇️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

justinchuby added a commit that referenced this pull request Aug 22, 2026
…#1765)

`Mobius metadata packages` currently **cannot pass for anyone**, on any
PR, regardless of content.

```
git fetch ... +refs/heads/justinchuby/catalogue-diffusion-builds*:...
The process '/usr/bin/git' failed with exit code 1   (x3, then error)
```

`.github/workflows/mobius-producer-conformance.yml:28` checks out
`onnxruntime/mobius` at `justinchuby/catalogue-diffusion-builds`. `git
ls-remote` confirms that branch no longer exists. The job triggers on
every `pull_request`, so it is a blanket false red — it is the **only**
failure on #1747, and one of the reds on #1764 and #1729.

## Why now

The step's own comment stated its exit condition:

> `# Stacked with mobius#538 until its image/video workflow producer
lands.`

**mobius#538 merged today at 16:53 UTC** ("Support compact SD and
complete video diffusion workflows") and its branch was deleted on
merge. That is why this went red today rather than degrading gradually.
The stacking condition is satisfied, so the pin comes off.

## Verified, not assumed

I did not just swap the ref and call it fixed — pointing at a branch
that lacks the producer would only trade a fetch failure for a script
failure.

- **mobius#538's merge commit `b9c90433` compares `identical` to
`mobius/main`** — `ahead_by: 0`, `behind_by: 0`. So following `main`
today fetches byte-equivalent content to what the deleted branch
carried. This is a ref change, not a content change.
- **All three paths the job consumes exist at `mobius/main`:**
`tests/generate_onnx_genai_validation_packages.py`,
`tests/compare_onnx_genai_validation_packages.py`, and the
`tests/fixtures/onnx_genai_workflows` fixture directory.
- YAML parses; the resolved ref is `main`.

The real verification is this PR's own run of the job, since the failure
is entirely in CI and cannot be reproduced locally. If it does not go
green here, the diagnosis was incomplete and I would rather find that
out on this PR than on someone else's.

## Tradeoff, stated rather than hidden

`ref: main` floats. An upstream commit can redden this repo with no
local change — the same class of externally-caused red I am removing
here.

A SHA pin would eliminate that, at the cost of the job no longer
conforming against the *current* producer, which is the entire purpose
of a producer-conformance check. Tracking `main` is the intent of the
job, and its failure mode is an actionable red rather than a permanent
one. If the upstream churn turns out to be noisy in practice,
pinning-with-a-bump-process is the fallback, but that should be driven
by observed churn rather than pre-emptively.

## Not addressed

This does not touch the other reds on #1764 (inherited `E0599` from the
`RegisteredMemoryContext::device` breakage, fixed on main by #1762 —
those need a rebase) or #1729 (six genuine reds across Windows, macOS,
and CUDA).

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c80f8522-983c-47f7-8241-2155a823aabe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant