Drop anvl suggests - #245
Merged
Merged
Conversation
pjrt suggested anvl so that tests/testthat/test-dispatch.R could drive the
dispatcher through anvl::jit(). That inverted the dependency: a pjrt feature
could not be tested without pjrt's own downstream package, and the tests
silently skipped wherever anvl was absent.
The file already contained everything needed to avoid that. Its own fixtures --
parr(), qarr(), pjrt_entry(), oav(), new_dispatcher() -- build an "AnvlArray"
and a compile-callback result by hand, and 17 of its tests already used them.
The other 17 now do too: each builds a dispatcher() with a callback of its own
and a tiny stablehlo program compiled on the spot, so the suite exercises the
engine against pjrt's own fixtures and loads nothing from anvl. Verified by
asserting "anvl" never enters loadedNamespaces() during the run.
Three things changed rather than being ported one-to-one:
* The cache-key tests (identical() keying, closure environments, bitwise
+0/-0, NA_integer64_) moved to the closure engine. Static-ness is resolved
in the dispatcher core before any engine is consulted, so the coverage is
the same -- but they now need no compiled program and no plugin, and stop
being skipped where one is missing.
* The pjrt engine's move_inputs cross-device copy gets a test. It had none:
it was reachable only through anvl::jit(device = ), and the closure-engine
move_inputs test beside it does not exercise a copy, because there pjrt
deliberately moves nothing and r_fun places its own inputs.
* The twelve jit()-level tests moved to anvl (test-jit-dispatch.R there).
They were integration tests all along -- that anvl's real callback and real
arrays match this engine -- and anvl imports pjrt, so that is the side of
the dependency that can hold them. Two were dropped as duplicates of
anvl-free tests already here, and the quickr one only ever tested anvl's
backend wiring.
The fixtures move to the top of the file, since every test now uses them.
What this does not change: the dispatcher's C++ still names anvl's data model
-- "AnvlArray", $data/$backend, the "plain" tag, the AnvlDtype vocabulary. That
is a contract pjrt defines and anvl produces, and it is not a package
dependency. AGENTS.md now says so, and says pjrt must not reacquire one.
stablehlo suggests pjrt, so pjrt suggesting stablehlo closed a cycle. Nothing in the package needed it: no R/, src/, tests/, man/ or vignettes/ file uses `stablehlo::` at all. The MLIR the tests compile is written as raw strings, and the many "stablehlo" mentions elsewhere are op names in those strings or prose in comments, not the package. The one real user is tools/stress-cpu-memory.R, a development stress script that is not part of the package's own R code, so R CMD check does not consider it when resolving declared dependencies. With this and the anvl removal, pjrt suggests only callr, testthat and xml2 -- nothing that depends on pjrt in turn.
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
1 participant
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.
No description provided.