dasLLAMA: the engine refuses a bare job queue, and every queue in the tree is configured - #3943
Conversation
894354e to
1f8a5ef
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new tripwire test’s subprocess command construction and the user-facing panic/doc wording need small adjustments to avoid flakiness and misleading guidance.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR makes dasLLAMA refuse “bare” job queues (created via with_job_que() but not configured via setup_dasllama_jobque()), because that regime is catastrophically slow for the engine’s fork/join dispatch; it adds an environment-variable escape hatch, and updates in-tree call sites so existing programs/tests/benches run on configured queues by default.
Changes:
- Add
get_jobque_fork_pool()to detect whether the current context is configured for fork-context pooling, and use it to tripwire bare-queue engine dispatch. - Introduce
DASLLAMA_ALLOW_BARE_JOBQUEto downgrade the tripwire from panic to warning, and document the knob. - Update loaders/tools/tests/benches/harness probes to call
setup_dasllama_jobque*()insidewith_job_que()blocks; add a model-free tripwire test.
File summaries
| File | Description |
|---|---|
| utils/dasllama-convert/main.das | Configures the job queue in tool-local with_job_que() blocks to avoid bare-queue dispatch. |
| src/builtin/module_builtin_jobque.cpp | Adds get_jobque_fork_pool builtin and binds it into the jobque module. |
| include/daScript/simulate/aot_builtin_jobque.h | Exposes the new get_jobque_fork_pool symbol to AOT-generated code. |
| doc/source/stdlib/handmade/function-jobque-get_jobque_fork_pool-0x24aa9e9140117fba.rst | Documents the new jobque query builtin. |
| doc/reflections/das2rst.das | Classifies get_jobque_fork_pool in the das2rst jobque docs grouping. |
| modules/dasLLAMA/dasllama/dasllama_par.das | Implements the bare-queue tripwire (panic/warn) at first counted dispatch. |
| modules/dasLLAMA/dasllama/dasllama_env.das | Adds allow_bare_jobque to the EngineEnv environment configuration. |
| modules/dasLLAMA/ENVIRONMENT.md | Documents DASLLAMA_ALLOW_BARE_JOBQUE. |
| modules/dasLLAMA/dasllama/dasllama_load.das | Ensures internally-spawned loader job queues are configured. |
| modules/dasLLAMA/dasllama/dasllama.das | Updates public API docs to mention the new job-queue configuration requirement. |
| modules/dasLLAMA/tests/test_ple_modes.das | Configures job queue before engine use in this test’s with_job_que() block. |
| modules/dasLLAMA/tests/test_forward.das | Configures job queue before engine use across multiple with_job_que() blocks. |
| modules/dasLLAMA/tests/test_batch_decode.das | Configures job queue before engine use across multiple with_job_que() blocks. |
| modules/dasLLAMA/tests/test_parity_pregate.das | Ensures the slice cell’s with_job_que() configures the queue; adds required module. |
| modules/dasLLAMA/tests/test_model_image_vulkan.das | Configures job queue for the cached-load with_job_que() block. |
| modules/dasLLAMA/tests/test_jobque_tripwire.das | Adds a model-free test that asserts panic/warn/silent behavior for bare vs configured queues. |
| modules/dasLLAMA/tests/_jobque_tripwire_root.das | Adds the spawned fixture program used by the tripwire test. |
| modules/dasLLAMA/tests/run.das | Registers the new tripwire test in suite selection lists. |
| modules/dasLLAMA/tests/CLAUDE.md | Documents the new tripwire test and the “always setup inside with_job_que” rule for tests. |
| modules/dasLLAMA/benchmarks/matmul/test_matmul_par.das | Configures job queue for threaded matmul tests. |
| modules/dasLLAMA/benchmarks/matmul/bench_matmul_par.das | Configures job queue for the matmul benchmark driver. |
| modules/dasLLAMA/harness/tq4_order_probe.das | Configures job queue in the probe’s with_job_que() block. |
| modules/dasLLAMA/harness/tq4_basis_probe.das | Configures job queue in the probe’s with_job_que() blocks. |
| modules/dasLLAMA/harness/residency_ramp_probe.das | Requires the setup helper and configures the job queue for the probe. |
| modules/dasLLAMA/harness/quant_eval.das | Requires the setup helper and configures the job queue for the harness run. |
| modules/dasLLAMA/harness/quant_eval_q4.das | Requires the setup helper and configures the job queue for the harness run. |
| modules/dasLLAMA/harness/gen_tune_probe.das | Configures job queue before using job-queue-dependent tuning/dispatch behavior. |
| modules/dasLLAMA/ARCHITECTURE_RUNTIME.md | Documents the new “configured queue only” policy and its performance rationale. |
| skills/internal/make_pr.md | Updates PR-body format guidance (Why/What changes/Observable behavior/Where to look). |
Review details
- Files reviewed: 29/29 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
1f8a5ef to
aff79b4
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new diagnostics/docs contain a couple of concrete accuracy issues (wording/claims) that should be corrected to match real call paths and existing configuration sites.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
modules/dasLLAMA/dasllama/dasllama_par.das:63
- The panic/warning text says to call setup_dasllama_jobque() before the first generate/eval, but this tripwire can also fire during other engine dispatches (e.g., load/conversion work that uses maybe_parallel_for). Widening the wording would make the diagnostic accurate regardless of which call triggers the first dispatch.
let msg = "dasLLAMA: the job queue is not configured for the engine's fork/join dispatch - call setup_dasllama_jobque() inside with_job_que() before the first generate/eval (a bare queue decodes ~200x slower; DASLLAMA_ALLOW_BARE_JOBQUE=1 runs anyway)"
modules/dasLLAMA/dasllama/dasllama.das:15
- This docstring says the setup call is needed before the first generate/eval, but the job-queue tripwire can also fire during load_model (dasllama_load.das uses maybe_parallel_for during conversion), so callers wrapping load in with_job_que() will hit the panic earlier than this text implies.
//! call ``setup_dasllama_jobque()`` inside it before the first ``generate``/``eval`` — the engine
- Files reviewed: 29/29 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The behavior change is guarded, well-documented, and backed by a focused regression test, with only minor doc-comment readability feedback.
Review details
- Files reviewed: 29/29 changed files
- Comments generated: 1
- Review effort level: Lite
… tree is configured
A queue as create_job_que makes it clones a fork context per job, wakes one worker per push and parks
each worker the moment its job ends; the engine's ~160 dispatches per decoded token turn that into an
E2B q8 forward step of 3.6 s against 16 ms configured. Half the stocked suite's wall was this regime:
test_ple_modes alone ran 485 s and now runs 37 s.
- jobque module: get_jobque_fork_pool() reads the per-context pool flag - set_jobque_fork_pool is the
one knob setup_dasllama_jobque() always turns on, and the flag outlives the queue, so it is the
"configured" observable
- dasllama_par: the first counted dispatch of a process checks the queue once; a queue without the pool
panics naming setup_dasllama_jobque(), DASLLAMA_ALLOW_BARE_JOBQUE=1 (EngineEnv.allow_bare_jobque)
downgrades that to a warning for a run measuring the bare regime; no queue at all is not checked.
The fact is ARCHITECTURE_RUNTIME.md 2.20 {#jobque-policy}, cited from the check
- the loader's own scoped queues (load_gguf, load_gguf_streaming) configure themselves; the facade header
says to call the setup inside with_job_que before the first generate/eval
- every bare block in the tree gets the setup: dasllama-convert (2), test_ple_modes, test_batch_decode
(9), test_forward (4), test_parity_pregate, test_model_image_vulkan, bench_matmul_par, test_matmul_par
(2), quant_eval, quant_eval_q4, gen_tune_probe, residency_ramp_probe, tq4_basis_probe (2),
tq4_order_probe
- test_jobque_tripwire.das: a bare child panics and names the call, the env knob lets it run with the
warning, a configured queue dispatches silently; registered model-free and in the llm area
- ENVIRONMENT.md regenerated; handmade rst for the builtin
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…anges, observable behavior, where to look Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
aff79b4 to
3796b34
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The behavior change is covered by a dedicated tripwire test and systematic call-site updates, with only minor documentation-consistency nits remaining.
Review details
- Files reviewed: 29/29 changed files
- Comments generated: 2
- Review effort level: Lite
Behavior change: a dasLLAMA program that opens
with_job_que()withoutsetup_dasllama_jobque()now panics at its first engine dispatch.DASLLAMA_ALLOW_BARE_JOBQUE=1keeps it running with a warning.Why. A queue nobody configured clones a fork context per job and parks each worker after it. The engine issues ~160 dispatches per decoded token, so an E2B q8 forward step took 3.6 s on such a queue against 16 ms configured.
test_ple_modesalone ran 485 s on this, and half the stocked suite's wall was the same regime.What changes.
get_jobque_fork_pool()(jobque module) reads the per-context pool flag. It is the one knobsetup_dasllama_jobque()always sets, nothing else sets it, and it outlives the queue.check_jobque_configuredindasllama_par.das). A queue without the pool panics naming the setup call; the env knob turns the panic into a warning. No queue at all is not checked.load_gguf,load_gguf_streaming) configure themselves. Every bare block in the tree gets the setup call: dasllama-convert (2), six test files, two matmul benches, eight harness probes.skills/internal/make_pr.md: the PR body's top layer is four labeled parts (Why, What changes, Observable behavior, Where to look), ~100 words, bullets only.Observable behavior.
test_ple_modes: 485 s -> 34 s.test_batch_decode: 264 s -> under 30 s. Stocked suite: 1577 s -> 1018 s.dasllama-convert: conversion passes ran on a bare queue -> configured queue.Where to look.
check_jobque_configuredindasllama_par.das; the two loader blocks indasllama_load.das;ARCHITECTURE_RUNTIME.mdsection 2.20.Validation, claims, ledger
Validation
DASLLAMA_PARITY_FULL=1) did not run; their wall under the tripwire is unmeasured.preflight --fullgreen after one fix: the new builtin joined the jobque page's "Internal invocations" group in das2rst.Claims - stated, not tested
bench_matmul_parthat gained the setup call compile on all lint rails but were not run.Not done
DASLLAMA_PARITY_FULL=1stocked run under the tripwire.compile_fileruns past the AST module cache.🤖 Generated with Claude Code