fix[next]: stop KeyError from masking compiled-program failures - #2731
fix[next]: stop KeyError from masking compiled-program failures#2731havogt wants to merge 2 commits into
Conversation
`CompiledProgramsPool.__call__` did all cache-miss handling inside the `except KeyError` handler of the dispatch lookup. Anything raised there — a failed artifact load, a compilation error re-raised from a worker — was chained to the `KeyError`, so the traceback led with an opaque key tuple instead of the actual cause. Leave the handler before handling the miss, so the miss path runs with a clean exception context, and wrap `artifact.load()` failures in an error naming the program. The hit path is unchanged: `try: d[key] except KeyError:` is kept and the `else` branch compiles to the same bytecode as before.
egparedes
left a comment
There was a problem hiding this comment.
I would go further in the simplification of the code here
| try: | ||
| compiled_program = self.compiled_programs[key] | ||
| except KeyError: | ||
| # The handler is empty on purpose: everything that could raise while it is active | ||
| # would be chained to this `KeyError`, whose opaque key then dominates the traceback | ||
| # and hides the actual failure. Leaving the handler clears the exception context. | ||
| pass | ||
| else: | ||
| with compiled_program_call_context(self, key, args, kwargs, offset_provider): | ||
| compiled_program(*args, **kwargs, offset_provider=offset_provider) | ||
| return |
There was a problem hiding this comment.
It would be even better to get rid completely of the try-except. I don't think the minimal saving in python overhead is worth the convoluted code anymore:
| try: | |
| compiled_program = self.compiled_programs[key] | |
| except KeyError: | |
| # The handler is empty on purpose: everything that could raise while it is active | |
| # would be chained to this `KeyError`, whose opaque key then dominates the traceback | |
| # and hides the actual failure. Leaving the handler clears the exception context. | |
| pass | |
| else: | |
| with compiled_program_call_context(self, key, args, kwargs, offset_provider): | |
| compiled_program(*args, **kwargs, offset_provider=offset_provider) | |
| return | |
| compiled_program = self.compiled_programs.get(key) | |
| if compiled_program is None: | |
| self._dispatch_miss( | |
| key, args, kwargs, canonical_args, canonical_kwargs, offset_provider, enable_jit | |
| ) | |
| with compiled_program_call_context(self, key, args, kwargs, offset_provider): | |
| compiled_program(*args, **kwargs, offset_provider=offset_provider) |
There was a problem hiding this comment.
Done in 95b66d3 — agreed, the ns aren't worth the convoluted control flow.
One correction to the snippet: as written it falls through to compiled_program(...) with compiled_program still None after _dispatch_miss, because that helper re-dispatched through self(...) and returned. But that's moot now — without a KeyError handler there is no exception context to escape, so _dispatch_miss is gone entirely and the miss branch went straight back inline where it was. The diff against main is now just the .get() + if, the error-message change, and _load_artifact.
Measured cost of the switch (isolated dict lookup, key present):
| 3.10 | 3.12 | 3.13 | |
|---|---|---|---|
try: d[key] except KeyError: |
30.0 ns | 15.7 ns | 16.1 ns |
d.get(key) |
30.6 ns | 21.7 ns | 20.1 ns |
~5 ns on >=3.11 (free on 3.10), against ~2800 ns for a full dispatch.
Addresses review feedback: a plain `.get()` never raises, so there is no exception context that could swallow the real failure, and the cache-miss branch goes back inline where it was — no helper method, no early return. Costs ~5ns per call on Python >=3.11 (~20ns vs ~16ns for the lookup itself, out of ~2800ns total dispatch), and nothing measurable on 3.10.
|
Superseded by stack #2735: #2733 (the fix) and #2734 (the error-message improvement), split per @egparedes' review and so the message part can be dropped independently. The |
Description
Draft — opening early to discuss the trade-offs below.
When a compiled program cannot be obtained, the traceback leads with a
KeyErroron thedispatch dict even though the actual cause is something else. Real incident (ICON/icon4py,
10 MPI ranks, shared OTF cache on Lustre scratch):
The genuine cause was
OSError: [Errno 116] Stale file handlewhile loading the artifact, butthe
KeyErrorwith its opaque key tuple is what everyone reads first — it looks like acache-key/dispatch bug, and it is not. That cost hours of misdirected debugging.
CompiledProgramsPool.__call__did all cache-miss handling inside theexcept KeyErrorhandler, so everything raised there —
future.result()re-raising a worker exception,artifact.load()doing I/O — was chained to theKeyError.Changes
self.compiled_programs.get(key)plus anif ... is None. Noexception is raised on a miss, so there is no context for the real failure to be chained to.
The miss branch itself is unchanged.
_load_artifactwrapsartifact.load()failures asRuntimeError("Failed to load the compiled program '<name>'.") from <cause>, so theOSErroris what the reader sees. Worker exceptions from
future.result()are deliberately notwrapped — they propagate as themselves.
(
... of 'diffusion_run': scalar_int=3) and notes that a variant is also selected by theidentity of the
offset_providerentries.Hot path
__call__runs per program invocation (thousands per second in ICON), so the cost of thesuccessful lookup matters. Isolated dict lookup, key present:
try: d[key] except KeyError:d.get(key)key in d+d[key]So
.get()costs ~5 ns on ≥3.11, where the non-raisingtrybecame free, and nothing on 3.10.That is 0.2 % of the ~2800 ns a full dispatch takes; a whole-dispatch microbenchmark (program
call with the executable replaced by a no-op, under
-O) cannot resolve it above run-to-runnoise. Taken deliberately in exchange for a much smaller, non-convoluted diff
(review thread).
An earlier revision kept the
try/exceptand restructured__call__so the handler was leftbefore any miss handling (identical hit-path bytecode, zero cost). That version is in the
history of this branch if we ever want the ns back.
Note that just adding
raise ... from eat the raise sites does not fix this:from esets__suppress_context__on the outer exception, but the intermediateOSErrorstill carries__context__ = KeyErrorand is printed as the cause, so theKeyErrorstill leads thetraceback. Hiding it that way would require setting
inner.__context__ = Noneat every site.Open for discussion
message improvement, not part of the fix. Drop it?
CompiledProgramNotAvailableError) carrying the key and the causekind was considered and skipped — nothing catches these today.
_finish_compilation_jobstill hasassert key not in self.compiled_programs, which neverruns under icon4py's
PYTHONOPTIMIZE=2. Left as an assert: it is only reachable right after aconfirmed dict miss, so it cannot be violated by user input.
Out of scope: the underlying Lustre
ESTALE, which belongs with the OTF cache write/replacelogic (#2691). This is only about which error the user is shown.
Requirements
Three unit tests in
tests/next_tests/unit_tests/otf_tests/test_compiled_program.py(load
OSError, worker exception, genuine miss); all three fail onmainand pass here.