Repository navigation
Add available_engines() and active_engine() introspection functions; treat the reference implementation as an engine - #25
Draft
Jim Garrison (garrison) wants to merge 5 commits into
Draft
Jim Garrison (garrison) wants to merge 5 commits into
Jim Garrison (garrison) wants to merge 5 commits into
Conversation
These are the two functions anticipated when enable_engine became a free function: the argument for that shape was that a domain author should not have to re-export each new user-facing function, so these follow it. available_engines(domain) returns a sorted tuple of the engine names that can be enabled. It unions the domain's engine registry with the names advertised through the coheriq.engines.<domain> entry point group, because enable_engine accepts either. Advertised plugins are deliberately not imported to answer the question, so a listed name means enable_engine will attempt it, not that it is guaranteed to load. active_engine(domain) returns the name of the enabled engine, or None. None covers both a domain that has not resolved its implementation yet and one already frozen on its defaults: "no engine is active" is equally true of both, and collapsing them keeps the return type str | None. Neither function resolves or freezes the implementation. This is the deliberate contrast with calling a candidate: asking which engine is active must not commit the domain to an answer, since that would make an introspection call silently foreclose enabling an engine afterwards. A sorted tuple rather than a set: these results are usually shown to a person, and set iteration order for strings varies between processes, which would make documentation examples and error messages non-reproducible. Also factors the domain-or-name coercion out of enable_engine into a shared _resolve_domain helper, now that three public functions accept that same argument. Adds a release note; this is the first note in releasenotes/notes/. Assisted-by: Claude Opus 5
The previous signature returned None in two distinct situations: the domain had not resolved an implementation yet, and the domain had resolved to its own default implementation. Those are opposite answers from the caller's point of view. In the first case an engine can still be enabled; in the second it is permanently too late. Overloading None meant a caller could not tell the actionable state from the settled one. The function now returns None only for "not decided yet", and the new REFERENCE constant (the string "reference") when the domain has resolved to the implementation it provides itself. Any other string is an engine name. Since None no longer conflates the two, a None result cannot mislead a caller into thinking the decision is still open when it is closed. Naming the resolved default "reference" follows the usual HPC sense of a reference implementation, and it is what makes the rename appropriate: the function reports which implementation is in effect, and the domain's own implementation is one of the possible answers, so "engine" was too narrow a word for the return value. Because "reference" is now a possible result, it can no longer also be an engine name; AccelerationEngine rejects it via a _RESERVED_ENGINE_NAMES denylist. The denylist is a compatibility hazard for any engine already using a name added to it, so it is kept deliberately small and the comment says so. active_implementation() classifies the domain states explicitly rather than treating "not ENGINE_ENABLED" as the undecided case, so that adding a state later forces a deliberate decision about how it is reported. Assisted-by: Claude Opus 5
The dispatch machinery has always treated a domain's own implementations as an engine. materialize() builds a class from the domain's candidates, and every engine builds a class that inherits from it, so the reference implementation is the root of the engine inheritance graph rather than a separate kind of thing beside it. Resolving a call reads an attribute off whichever class is active, with no branch for the two cases. Only the public API disagreed, and this makes it match. The reference implementation is now addressable by name. REFERENCE is no longer a sentinel meaning "no engine"; it is the name of the engine every domain has. So: - enable_engine(domain, REFERENCE) selects it explicitly. - available_engines() lists it, first, ahead of the other engines. - active_implementation() goes back to being active_engine(), because "reference" is now an engine name and the function no longer has to report something that isn't one. Being able to ask for it explicitly is the substantive gain, not just the naming. Declining to choose an engine and choosing the reference implementation both end with it active, but they are different instructions: an environment variable overrides the first and must not override the second. Someone comparing against the reference implementation, debugging a suspected engine bug, or pinning known-good behavior in a test needs to say so in a way that silence cannot. Explicit activation already takes precedence over the environment variable, so this inherits that rule rather than adding one. Pinning the reference implementation is a forward transition into the same settled state a first call would reach. It forecloses the other engines, just as enabling an engine forecloses the alternatives, and it cannot undo an engine that is already active -- so the one-way contract is unchanged. Enabling it twice, or after a call has already settled on it, is an idempotent no-op, matching enable_engine() on an already-active engine. _RESERVED_ENGINE_NAMES survives with a different justification. The name was previously refused because it meant something other than an engine; it is now refused because it names an engine the domain itself owns, which a third party cannot claim. available_engines() therefore never returns an empty tuple, so its length is not a count of the accelerators installed. The docstring says so, since the emptiness check is the plausible thing for a caller to have written. Assisted-by: Claude Opus 5
Coverage Report for CI Build 37349215726Coverage increased (+4.0%) to 90.0%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Jim Garrison (garrison)
changed the title
available_engines() and active_engine() introspection functions; treat the reference implementation as an engine
The architecture notes did not mention REFERENCE, enable_engine(domain, REFERENCE), available_engines(), or active_engine(), so an agent reading them would not know the API exists or that a domain's own implementations are reachable as an engine. The three-party model also said materialize() "locks in the default implementations". What it does is build the class that is the domain's reference implementation, and "default" is the ambiguous word here: if a domain ever ships an engine that is used unless something else is enabled, the default becomes that engine while the reference implementation stays what it is. Assisted-by: Claude Opus 5
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
2 participants
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.
An alternative to #14, which it supersedes if we take this direction. It contains #14's two commits unchanged and adds one on top (f3b2002), so the diff against #14 is just that commit.
The question this answers is whether a domain's own implementations count as an engine. #14 said no:
REFERENCEwas a sentinel meaning "no engine is active", andAccelerationEnginerejected the name because it meant something other than an engine. This says yes.Why yes
The dispatch machinery already treats it that way.
materialize()builds a class from the domain's candidates, and every engine builds a class that inherits from it, so the reference implementation is the root of the engine inheritance graph rather than a separate kind of thing beside it. Resolving a call reads an attribute off whichever class is active, with no branch for the two cases. Only the public API disagreed.The documentation's existing definition also fits it already: an engine is defined by what is substituted — a coherent unit of behavior, state, and invariants, chosen as a whole — not by who wrote it or how fast it is.
What changes
enable_engine(domain, REFERENCE)selects the reference implementation explicitly.available_engines()lists it, first, ahead of the other engines.active_implementation()goes back toactive_engine(), since"reference"is now an engine name and the function no longer reports something that isn't one.REFERENCE's docstring describes it as the name of an engine every domain has, rather than as a sentinel._RESERVED_ENGINE_NAMESsurvives with a different justification: the name is refused because the domain owns it, not because it means "not an engine". The error message now says "reserved for the domain's own use".The substantive gain is explicit selection, not the naming
Declining to choose an engine and choosing the reference implementation both end with it active, but they are different instructions: an environment variable overrides the first and must not override the second.
Someone comparing against the reference implementation, debugging a suspected engine bug, or pinning known-good behavior in a test needs to say that in a way silence cannot. Explicit activation already takes precedence over the environment variable, so this inherits the existing rule rather than adding one. This also closes #17.
It does not weaken the one-way contract
Pinning the reference implementation is a forward transition into the same settled state a first call would reach. It rules out the other engines in exactly the way enabling any engine rules out the rest, and it cannot undo an engine that is already active — enabling it after an engine is active raises
CoheriqDomainError. Enabling it twice, or after a call has already settled on it, is an idempotent no-op, matchingenable_engine()on an already-active engine.Behavior change worth noting
available_engines()never returns an empty tuple now, so its length is not a count of the accelerators installed. A caller checkingif not available_engines(d)to mean "nothing installed" would silently stop working. The docstring says so explicitly, since that check is the plausible thing to have written. Nothing in this repository outside the test suite made that check.Implementation note
domain._engine_registryhas no"reference"entry, because the domain builds that implementation rather than anAccelerationEngineregistering it.enable_enginetherefore special-cases the name ahead of the registry lookup and calls a new_enable_reference_low_level, which performs the same freeze a first call would. The alternative — having the domain construct a realAccelerationEnginefor itself atmaterialize()— is more uniform but touches more code and makes the self-engine inherit from the class it is built from. The special case seemed the better starting point; I am not certain it is the right long-term shape.Testing
tox -e py— 64 passed (50 before this commit)tox -e lint,tox -e style,tox -e doctest,tox -e docs— greentox -e ragged— 21 passedNew
TestEnableReferencecovers: pinning by constant and by literal string; accepting a domain name; precedence over the environment variable; ruling out other engines afterwards; idempotence, both after an explicit enable and after an implicit resolution; rejection once an engine is active; rejection beforematerialize(); and that every nameavailable_engines()reports can in fact be passed toenable_engine().TestAvailableEnginesgains tests that the reference entry is present, is listed first even ahead of names that sort before it alphabetically, and that the result is never empty.Notes
active_enginecross-reference as plain text rather than a link, because that function does not exist onmainyet; it resolves once this merges.available_engines()by engine priority rather than alphabetically, with the reference entry as the ordering placeholder. Not attempted here.This pull request was generated by Claude Opus 5 under my guidance.