Repository navigation
Add coheriq.resolve_impl() to reach a candidate's resolved implementation - #15
Draft
Jim Garrison (garrison) wants to merge 3 commits into
Draft
Jim Garrison (garrison) wants to merge 3 commits into
Jim Garrison (garrison) wants to merge 3 commits into
Conversation
…ntation A class or function marked with @domain.acceleration_candidate becomes a dispatching wrapper in user code, and the implementation it dispatches to is not otherwise reachable. For a class the gap is severe, because the wrapper is a function object standing where the class name used to be. Construction works, but nothing else a class can do does: isinstance() rejects the wrapper as its second argument, @classmethod alternate constructors raise "'classmethod' object is not callable", and __new__ and subclassing are unreachable. The example domain already works around this -- RaggedBatch.from_flat cannot be a @classmethod, so it exists as the free function ragged_batch_from_flat. For a function the wrapper forwards calls faithfully, so the gap is narrower but real: what it hides is the implementation's identity. Which object is actually being called is not answerable by calling it, and that is a fair question to ask -- to compare with "is", or to inspect the resolved function itself. get_active_impl(candidate) returns what the wrapper dispatches to: the domain default, or the enabled engine's override. It accepts either kind of candidate; restricting it to classes would withhold the identity of a function implementation for no benefit, since calling a function candidate already freezes the domain, so nothing is protected by refusing. Like calling a candidate, it resolves and freezes the implementation, which is why it is a function call rather than an attribute. __wrapped__ (already set by functools.wraps) remains the escape hatch for the original, ignoring any engine, with no such side effect. Deliberately a free function, not a method: mypy special-cases class decorators and will not substitute a decorator's return type onto the class symbol, so a method errors at every call site. A metaclass would work but conflicts with nanobind's metaclass for bound C++ types, which engines use. The signature is identity-preserving (_F -> _F, bound to Callable) rather than returning type[_T]. Because of the existing "if TYPE_CHECKING and isclass(f): return f" branch, a decorated class reads to a type checker as a constructor callable rather than type[...], and a decorated function matches that same shape -- so a type[_T] return would be correct for classes and simply wrong for functions. The identity signature is accurate for every input; the cost is that isinstance() against the result needs a cast until a type[_T] overload is worth adding. Note that coheriq does not ship a py.typed marker, so the type-level behavior above is dormant downstream until it does. The runtime motivation is live today regardless. No get_default_impl() counterpart: the default is only referenceable from within default code, but that code only runs when the default is active, where active == default already. The docs entries for get_active_impl and CoheriqTypeError are new here. Adds a release note. Assisted-by: Claude Opus 5
Two problems with the old name. It read as a plain accessor, but the call is not one: like calling a candidate, it resolves and freezes the implementation, after which no engine can be enabled. A "get_" prefix suggests a side-effect-free read, so the name worked against the docstring that had to warn about the side effect. "resolve" carries it in the verb, and matches the vocabulary the code already uses for this operation -- _ensure_realized_impl, _resolve_domain, and the docstrings describing a call as resolving the implementation. It was also confusable with active_implementation(), which is a near-identical name for a function returning something different in kind: that one returns an implementation's *name*, this one returns the implementation object. The echo between the two names implied a kinship that misrepresented the return types. Dropping it removes the near-miss pair rather than making it tighter, which is what naming this resolve_implementation() would have done. "impl" is kept rather than spelled out, since it is what the code calls the object throughout, and it distinguishes the object-returning function from the name-returning one. The wrapper attribute carrying the hook is renamed to _coheriq_resolve_impl to match. It is private, but leaving it named after the old function would strand the vocabulary at the one place a reader goes to understand how the public function reaches the implementation. The release note file is renamed for the same reason; it is the only note on this branch and has not been released, so its slug is free to change. Assisted-by: Claude Opus 5
Coverage Report for CI Build 36934638601Coverage increased (+1.1%) to 87.168%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
coheriq.resolve_impl(), so the implementation an acceleration candidate dispatches to can be reached directly.Reviewable as two commits: the first adds the function, the second renames it from
get_active_impl()for the reasons under Whyresolve_implbelow.The problem
A class or function decorated with
@domain.acceleration_candidateis replaced by a dispatching wrapper. For a class, the wrapper is a function object standing where the class name used to be, so construction works and nothing else does:__new__, subclassing, and introspection are unreachable for the same reason. This matters because swapping a Python data structure for a compiled one is a headline use case for this package, so class candidates aren't a corner case. The existing example already works around it:RaggedBatch.from_flatcan't be a@classmethod, so it lives as the free functionragged_batch_from_flat.For a function candidate the wrapper forwards calls faithfully, so the gap is narrower but real: what it hides is the implementation's identity. Which object is actually being called isn't answerable by calling it.
The change
resolve_impl(candidate)returns what the wrapper dispatches to — the domain default, or the enabled engine's override:Like calling a candidate, it resolves and freezes the implementation, so no engine can be enabled afterwards. That's why it's a function call and not an attribute — the side effect should look deliberate. It's the same side effect calling any candidate already has, not a new one.
__wrapped__(already set byfunctools.wraps) remains the escape hatch for the original, ignoring any engine, with no such side effect.Also adds
CoheriqTypeError(CoheriqUserError, TypeError), raised for a non-candidate. The dual inheritance means it's catchable both asCoheriqErrorand as a plainTypeError.Why
resolve_implThe function was originally
get_active_impl(). Two problems with that name, both fixed by the second commit:get_reads as a side-effect-free accessor, and this call isn't one. It freezes the implementation permanently. Naming itget_put the name at odds with the docstring, which has to warn about exactly that.resolvecarries the effect in the verb, and it's already this codebase's word for the operation —_ensure_realized_impl,_resolve_domain, and the docstrings that describe a call as resolving the implementation.available_implementations()-style name-returning functions, in particularactive_implementation()(Add available_engines() and active_implementation() introspection functions #14). That one returns an implementation's name; this returns the implementation object. The near-identical names implied a kinship that misrepresented the return types, so dropping the echo removes the collision — whereas spelling this oneresolve_implementation()would have tightened it.implis kept rather than spelled out: it's what the code calls the object throughout, and the abbreviation is what distinguishes the object-returning function from the name-returning one.The private wrapper attribute carrying the hook is renamed to
_coheriq_resolve_implto match, so the one place a reader looks to understand how the public function reaches the implementation doesn't still use the old vocabulary.It accepts both kinds of candidate
An earlier draft restricted this to classes, on the reasoning that a function wrapper already forwards everything that matters. That was too strong — identity is a fair thing to ask for, whether to compare with
isor to inspect the resolved function itself, and calling through the wrapper can never answer it.Restricting it also protected nothing: calling a function candidate already freezes the domain, so refusing the direct route just leaves the indirect one open.
Design notes
A free function, not a method. mypy special-cases class decorators and won't substitute a decorator's return type onto the class symbol, so a
RaggedBatch.resolve_impl()method errors at every call site. The alternatives were a mixin base (works, but imposes inheritance on domain authors) or a metaclass — rejected, because it conflicts with nanobind's metaclass for bound C++ types, and engines are exactly where nanobind shows up.The signature is identity-preserving (
_F -> _F, bound toCallable). Because of the existingif TYPE_CHECKING and isclass(f): return fbranch, a decorated class reads to a type checker as a constructor callable rather thantype[...]— and a decorated function matches that same shape. So atype[_T]return would be correct for classes and simply wrong for functions (it would claimtype[int]for a function returningint). The identity signature is accurate for every input. The cost is thatisinstance()against the result needs a cast; an overload can recover that later if it's worth it.No
default_impl()counterpart. The default implementation is only ever worth referencing from within default code — but that code only runs when the default is the active implementation, where active and default are the same thing. For the genuine remaining case (introspection, tests),__wrapped__already covers it.Caveat worth flagging
coheriqdoesn't ship apy.typedmarker, so downstream consumers currently see everything imported fromcoheriqasAny— which means the type-checking discussion above is dormant until that changes. I confirmed this empirically: from outside the package both calls revealAnyregardless of the signature. Adding the marker is tracked separately in #16 and isn't part of this PR.The runtime motivation (
isinstance/@classmethod/__new__being broken, and function identity being unreachable) is live today regardless.Testing
tox -e py— 36 passed, of which 12 are new (TestResolveImpl)tox -e lint— green: ruff, copyright check, mypy, pylint 10.00/10 onsrc/andtest/, nbqa, renotox -e doctest,tox -e docs— both greenNew tests cover: resolution to the default class;
isinstanceand__new__becoming usable; reflecting an active engine; the freeze side effect;__wrapped__staying the original class under an active engine; rejection of non-candidates including the dual-catchability of the error; and, for function candidates, identity against both the default and an engine override, agreement between calling the result and calling the wrapper, and the freeze.Notes
resolve_implandCoheriqTypeErrorare added to the API docs (user_functions.rst,exceptions.rst).get-active-impl, predating the rename. Left alone deliberately, to keep this PR's URL and review history.active_implementation, so that Add available_engines() and active_implementation() introspection functions #14 can land first without the two branches colliding. Restoring that xref is a follow-up on this branch once Add available_engines() and active_implementation() introspection functions #14 merges.ragged_batch_from_flatshould now revert to a@classmethod, to exercise this in the demo. Kept out to keep the diff focused.This PR was generated by Claude Opus 5 under my guidance.