Skip to content

Key topic_buffer_full by TP in both conductors - #764

Closed
wbarnha wants to merge 1 commit into
claude/faust-cython-conductor-parityfrom
claude/faust-topic-buffer-full-tp
Closed

Key topic_buffer_full by TP in both conductors#764
wbarnha wants to merge 1 commit into
claude/faust-cython-conductor-parityfrom
claude/faust-topic-buffer-full-tp

Conversation

@wbarnha

@wbarnha wbarnha commented Aug 7, 2026

Copy link
Copy Markdown
Member

Description

Monitor.topic_buffer_full is a Counter[TP], and two paths report into it: the pressure-high callback, which passes a TP, and the full-queue path, which passed the channel. The same partition therefore accumulated under two different keys depending on which path noticed the buffer was full — splitting its count, and adding a second /stats entry labelled by channel for a partition already listed by TP.

Both implementations had it, which is why it went unfixed for so long. The comment in faust/transport/conductor.py recorded the defect and explicitly declined to fix it:

The Cython twin in _cython/conductor.pyx has the same bug; fixing either alone would make them disagree, so the defect is only recorded here, not fixed.

With the parity suite from #763 in place that objection is gone — both are corrected here, together, and the suite holds them level.

The # type: ignore[arg-type] on the call goes away with it. mypy -p faust is clean without it, which is the type checker confirming the argument is now the one the sensor declares.

Note on what parity testing does not do

The conductor parity tests were green throughout — before and after. Both implementations passed the channel, so they agreed with each other perfectly while both were wrong. A differential test only finds divergence; a shared mistake is invisible to it.

So the coverage added here is deliberately not another comparison:

  • the full-queue parity test now records the sensor's argument rather than a call count, and asserts it equals the TP;
  • a new test drives a real Monitor through the full-queue path and asserts every key of topic_buffer_full is a TP. It is parametrised over both implementations rather than comparing them, and runs against the pure-Python conductor even when the extension is absent, since the defect was in both.

Verified by reverting both twins and confirming each new assertion fails:

AssertionError: on_topic_buffer_full must be given the TP ... Got: [<Topic: foo0@0x7f15fa6d0e10>]
AssertionError: Monitor.topic_buffer_full is a Counter[TP], but the cython conductor keyed it by ['Topic']

Verification

configuration result
extensions built (FAUST_REQUIRE_CYTHON=1) 2272 passed
extensions absent 2208 passed
free-threaded 3.14t, PYTHON_GIL=0 2276 passed
mypy -p faust clean (164 files)

flake8 / black / isort clean; docs build clean.

Note on the base branch

Based on claude/faust-cython-conductor-parity (#763), which is itself based on #762. The fix depends on that parity suite: correcting both twins at once is only safe because something now holds them level, and this PR extends that suite rather than duplicating it. Retarget down the stack as each lands.


Generated by Claude Code

`Monitor.topic_buffer_full` is a `Counter[TP]`, and two paths report into
it: the pressure-high callback, which passes a TP, and the full-queue
path, which passed the *channel*.  The same partition therefore
accumulated under two different keys depending on which path noticed the
buffer was full -- splitting its count, and adding a second `/stats`
entry labelled by channel for a partition already listed by TP.

Both implementations had it, which is why it went unfixed for so long:
the comment in faust/transport/conductor.py recorded the defect and
explicitly declined to fix it, because correcting one twin alone would
have made the two disagree.  With the parity suite in place that
objection is gone -- both are corrected here, together, and the suite
holds them level.

The `# type: ignore[arg-type]` on the call goes away with it; `mypy -p
faust` is clean without it, which is the type checker confirming the
argument is now the one the sensor declares.

## Note on what parity testing does not do

The conductor parity tests were green throughout, before and after.
Both implementations passed the channel, so they agreed with each other
perfectly while both were wrong.  A differential test only finds
*divergence*; a shared mistake is invisible to it.

So the coverage added here is deliberately not another comparison:

* the full-queue parity test now records the sensor's *argument* rather
  than a call count, and asserts it equals the TP;
* a new test drives a real `Monitor` through the full-queue path and
  asserts every key of `topic_buffer_full` is a TP.  It is parametrised
  over both implementations rather than comparing them, and runs against
  the pure-Python conductor even when the extension is absent, since the
  defect was in both.

Verified by reverting both twins and confirming each new assertion
fails: `Got: [<Topic: foo0@...>]` and `keyed it by ['Topic']`.

Suite green in every configuration: extensions built (2272 passed),
absent (2208 passed), free-threaded 3.14t under PYTHON_GIL=0 (2276
passed), and `mypy -p faust` clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K8qT5E3rnSXvw7ibNLXrVr

wbarnha commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

Folded into #762 — the whole series is now one branch, so there is no stack to land in order. The commit (9ed8f4b, topic_buffer_full keyed by TP in both conductors) is unchanged there.


Generated by Claude Code

@wbarnha wbarnha closed this Aug 7, 2026
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.

2 participants