Skip to content

fix(tests): prevent engine loop from calling step() in abort/cancelation tests#1324

Open
fry69 wants to merge 1 commit into
jundot:mainfrom
fry69:fry69/fix-test-engine-core
Open

fix(tests): prevent engine loop from calling step() in abort/cancelation tests#1324
fry69 wants to merge 1 commit into
jundot:mainfrom
fry69:fry69/fix-test-engine-core

Conversation

@fry69
Copy link
Copy Markdown
Contributor

@fry69 fry69 commented May 20, 2026

This fixes two new errors when running

pytest tests/test_engine_core.py

Please advise if this is the correct approach. For my still limited understanding about the codebase, it sounds plausible.

Below is the message from my coding agent:


MockModel lacks .layers attribute, causing scheduler.step() to fail with AttributeError from make_prompt_cache. Adding engine.scheduler.has_requests = lambda: False prevents the engine loop from running step() in tests that test abort/cancellation logic without needing model inference.

Fixes test_abort_request_wakes_blocked_stream_outputs and test_generate_cancel_aborts_request which were failing because the engine loop error handler interfered with expected error messages and collector cleanup.


Update: I asked the agent:

Would it be a better approach add the new .layers attribute to the MockModel to fix the root cause of this problem, rather than preventing the symptoms? Or is that a worse approach? Please explain.

It didn't like this approach much:


Both approaches have trade-offs. Here's the analysis:

Approach A — Add .layers to MockModel:

This fixes the make_prompt_cache crash at cache.py:34 but doesn't stop there. The prefill pipeline immediately continues to create mx.array(prefill_tokens), call self.model(input_arr) (which returns a MagicMock), then tries to extract logits, embeddings, etc. from that mock object. You'd need to mock ``.layers, .get_input_embeddings, the forward pass return type, and potentially more — it's a deep rabbit hole with fragile, high-maintenance mocks.

Approach B — has_requests = lambda: False (current fix):

These tests test abort/cancellation logic, not token generation. Preventing step() from running is semantically correct — the engine loop shouldn't be trying to generate tokens with a mock model. The pattern is already established at test_engine_core.py:586, so it's consistent.

Approach B is better here because it's simple, targeted, and doesn't require building fragile mock infrastructure that would need ongoing maintenance as the prefill pipeline evolves.

…tion tests

MockModel lacks .layers attribute, causing scheduler.step() to fail
with AttributeError from make_prompt_cache.  Adding
engine.scheduler.has_requests = lambda: False prevents the engine
loop from running step() in tests that test abort/cancellation
logic without needing model inference.

Fixes test_abort_request_wakes_blocked_stream_outputs and
test_generate_cancel_aborts_request which were failing because the
engine loop error handler interfered with expected error messages
and collector cleanup.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant