feat!: wire flashAttn up for real and follow the lazyMode rename - #198
Conversation
The knob has been documented and settable since it was introduced, and could never be forwarded. --flash-attn takes a mandatory on|off|auto, the binding offered only a bare-flag setter, and emitting the key alone made llama.cpp's parser consume the next argv token -- so the load died naming a flag the user never set. 1.2.0 shipped a refusal at plan time rather than that diagnostic. net.ladenthin:llama 5.2.0 adds ModelParameters.setFlashAttn(FlashAttn), so the provider now emits --flash-attn on when the knob is set. false keeps emitting nothing, which leaves llama.cpp's own auto default in force: that is what an unconfigured run should get, and it is what every other optional knob here does with its sentinel. Both refusals go, along with the three tests that pinned them, and the DEFAULT_FLASH_ATTN javadoc stops describing a knob that cannot be used. The history is kept in that javadoc rather than deleted -- the reason the value matters is exactly why the knob spent a release unusable. The part that is more than bookkeeping: flashAttn moves out of the knob sweep's NOT_SWEPT list into a real sweep case, so it is now set against a real model on every run. Until now only the refusal had ever been executed, which is what the removed TODO asked for. 36 sweep cases green, plus EngineSupport, AiGenerationConfig and the factory mapping -- 105 tests. llama.version moves to 5.2.0-SNAPSHOT deliberately: the binding change is not published yet, so CI stays red until it is. The CHANGELOG says so, so a red build is not mistaken for a regression.
GitHub Actions
Failure
GitHub Actions
Failure
Code Review SummaryThis is a well-executed fix that finally makes the ✅ Strengths
🔍 Observations
No bugs, security issues, or architectural concerns identified. |
BREAKING CHANGE: the tensorReadLazy model-definition field and its accessors are renamed to lazyMode. llama.cpp b10731 renamed --tensor-read-lazy to -lzm / --lazy-mode with no alias, and net.ladenthin:llama followed the rename instead of papering over it. Keeping tensorReadLazy here would leave the configuration surface naming a flag that no longer exists, so the field, its getters, the LlamaCppJniConfig accessor and builder, the factory mapping, module-info, the plugin README row and the sweep case all move. The mismatch this guards against is not cosmetic, and it showed up while doing the work: a new Java name paired with an old native fails at load time with "Failed to parse model parameters" and nothing earlier sees it. The knob sweep caught exactly that -- case [20] went red against a jar carrying the previous native, and green once the jar carried one built from the pinned tag. That is the second defect the sweep has found since it was written. Core module: 637 tests, 0 failures, 0 skipped, run against a libjllama.so built from b10731 with all eight patches applied.
GitHub Actions
Failure
GitHub Actions
Failure
GitHub Actions
Error
bernardladenthin
changed the title
Code Review SummaryThis PR makes two coordinated changes: fixing the ✅ Strengths
✅ Correctness
No issues detectedThe code is ready once the |
The provider now calls setFlashAttn(FlashAttn) and setLazyMode(LazyMode), neither of which exists in 5.1.0, so this module cannot compile against the released binding any more. Naming 5.2.0-SNAPSHOT would not help: this repo declares no <repositories> element (distributionManagement/snapshotRepository is deploy-only), so the Central snapshot channel is not even resolvable here -- and a -SNAPSHOT dependency on main would block every srcmorph release, since Central rejects a release that depends on one. The build therefore stays red until net.ladenthin:llama:5.2.0 is published, and goes green with no further edit the moment it is.
GitHub Actions
Failure
GitHub Actions
Failure
Review: Code Quality & CorrectnessThis is a well-executed PR. No issues found. Strengths
Verification✓ Null safety annotations present |
2 participants
This depends on
net.ladenthin:llama:5.2.0-SNAPSHOT, which is not published. CI will fail to resolve the dependency, and that is expected — the CHANGELOG says so too, so a red build here is not mistaken for a regression. Merge order: jllama#408 → a jllama release or snapshot deploy → this.1.
flashAttnworksThe knob has been settable and non-functional since it was introduced.
--flash-attntakes a mandatoryon|off|auto; the binding offered only a bare-flag setter, so emitting the key alone made llama.cpp's parser consume the following argv token and the load died naming a flag the user never set. 1.2.0 shipped a refusal at plan time rather than that diagnostic — the honest interim answer, not the fix.jllama 5.2.0 adds
ModelParameters.setFlashAttn(FlashAttn), so the provider now emits--flash-attn onwhen the knob is set.falsekeeps emitting nothing, leaving llama.cpp's ownautodefault in force — what an unconfigured run should get, and consistent with every other optional knob's sentinel here.Both refusals and the three tests pinning them are gone.
DEFAULT_FLASH_ATTN's javadoc stops describing an unusable knob but keeps the history: the reason the value matters is exactly why the knob spent a release refused.2.
tensorReadLazy→lazyMode— breakingllama.cpp b10731 renamed
--tensor-read-lazyto-lzm/--lazy-modewith no alias, and jllama followed the rename rather than papering over it. So the model-definition field, its accessors, theLlamaCppJniConfigaccessor and builder, the factory mapping,module-info, the plugin README row and the sweep case all move. Carrying the old name would leave the configuration surface describing a flag that does not exist.The part that is more than bookkeeping
flashAttnmoves out ofLlamaCppJniKnobSweepTest'sNOT_SWEPTlist into a real sweep case, so it is now set against a real model on every run. Until now only the refusal had ever been executed — closing the removed TODO's own condition ("actually set the knob against a real model once"). The sweep's completeness check is what forced this to be resolved rather than quietly left excluded.And the sweep found its second defect while this PR was being written. After the rename, case
[20](lazyMode) went red withFailed to parse model parameters— a new Java name paired with an old native, which fails only at load time and which nothing earlier in the pipeline can see. It went green once the jar carried alibjllama.soactually built from the pinned tag. That mismatch is a real hazard on every future rename, and it is now demonstrably caught.Test plan
Verified against a real binding, not assumed: jllama#408 installed into the local repository, with a
libjllama.sobuilt from b10731 with all eight patches applied (cmakeconfigure clean, full-O3build exit 0,NativeLibraryLoadSmokeTest4/4 including the pin cross-check).LlamaCppJniKnobSweepTest— every knob at a non-default value,flashAttn(true)andlazyMode("on")among themmvn compileclean under Error Prone / NullAway / Checker Framework;spotless:applyrunRelated issues / PRs
Consumer side of bernardladenthin/java-llama.cpp#408. Closes the
flashAttnentry inTODO.md, removed here.Checklist
CONTRIBUTING.mdandCODE_OF_CONDUCT.mdfeat!+BREAKING CHANGEtrailer)