Live Runner + Scope support - #20
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds gateway HTTP, discovery, runner selection, payment, media callback, Scope, and media rollover functionality. It adds echo, ping-pong, and text runner examples, plus tests, coverage settings, CI execution, and test documentation. ChangesGateway core
Runner examples
Validation and tooling
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant EchoClient
participant RunnerSelector
participant EchoRunner
participant MediaPublish
EchoClient->>RunnerSelector: reserve_session
RunnerSelector->>EchoRunner: POST /echo
EchoRunner->>MediaPublish: publish transformed frames
EchoClient->>EchoRunner: POST /update blur mode
sequenceDiagram
participant PingClient
participant RunnerSelector
participant PingRunner
PingClient->>RunnerSelector: discover runner
PingClient->>PingRunner: websocket ping timestamp
PingRunner-->>PingClient: pong and delta_ms
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| runner_url: str, | ||
| app: str, | ||
| price_per_unit: int = 0, | ||
| pixels_per_unit: int = 1, |
There was a problem hiding this comment.
@j0sh can we use something like unit_scale since this runner also allows different pricing schemes. Also see my pull request in go-liveper livepeer/go-livepeer#3942.
There was a problem hiding this comment.
I'll look into that idea (and the general move from away from pixels to purely timing) but don't want to block this merge for that
| secret: str, | ||
| runner_url: str, | ||
| app: str, | ||
| price_per_unit: int = 0, |
There was a problem hiding this comment.
@j0sh How do you intend pricing to work for dynamic runners? Right now the app self-asserts price_info via register_runner(price_per_unit=…) and go-livepeer trusts it (only > 0 is checked in normalizeHeartbeat). That's fine for operator-deployed/trusted containers — the operator sets the price via env and the app forwards it — but an untrusted image could ignore that and under-report its price. Static sidesteps this (operator sets price_info in runners.json); only dynamic trusts the app.
You can see how I'm currently using this in the hello_world example, which feels a bit strange since it relies on the app to create the argument and forward it.
Two thoughts:
- If the orchestrator is still meant to set the price in dynamic mode (to make gaming harder), the SDK could auto-read it from env (e.g. PRICE_PER_UNIT) instead of the app passing it to register_runner, keeping pricing a deployment/operator concern, out of app code.
- I think we'll eventually move to GPU-based pricing (orchestrator states a price per GPU type, workloads auto-run at that rate), so go-livepeer would override the reported price at registration anyway.
There was a problem hiding this comment.
That's correct, the runner itself reports the price because the orchestrator is intended to control the runner.
There was a problem hiding this comment.
This way runner provisioning (including pricing) can be configured separately from go-livepeer without having to introduce a mutual dependency on one other. Setting the price on go-livepeer itself introduces a tension with the orchestrator needing to be configured separately with details of the runner / workload, hardware, etc, as opposed to runners being able to simply connect and go.
If you want to keep the configuration / pricing within go-livepeer then static configuration is the way to go.
…ge into ja/live-runner (#46) ## Why The production SDK (`sdk-service:byoc-dual-path-1bf13cd`) carries a **load-bearing byoc-payment fix that exists in no branch** — only in the running container. That's an operational liability and it blocks a unified gateway. This ports it onto `ja/live-runner` (the Live Runner + Scope branch, PR #20 → main) so the consolidated gateway keeps BYOC payment working while gaining LR. ## What - `_payment_type_for_signer(signer_url)` — **legacy Daydream signer** (`signer.daydream.live`) → `type:"lv2v"` + string `capability`; **modern signers** (pymthouse DMZ, …) → `type:"byoc"` + BYOC capabilities protobuf. - `_create_byoc_payment` — assemble the payment payload + orch-discovery capabilities per the resolved type. - `capabilities.py` — `CapabilityId.BYOC` + `byoc_capabilities_from_app()`. Two files, +52/−9. Byte-identical to what runs in prod today. ## Relationship to #41 PR #41 sends `type:"byoc"` **unconditionally** and depends on an undeployed go-livepeer signer+orch change. Against `signer.daydream.live` (which only accepts `lv2v` today) that reproduces the **2026-07-13 “invalid job type” outage**. This PR is the **superset**: per-signer switching keeps the legacy signer working *and* enables modern signers. Recommend closing #41 in favor of this. ## Follow-on The same per-signer type logic generalizes to the **live-runner** payment path, which will let us drop the `lr-gateway` `lv2v` workaround once this lands. ## Test - [ ] `python -m py_compile` (passes locally) - [ ] `submit_byoc_job` against `signer.daydream.live` → `type:lv2v` → 200 (no regression) - [ ] `submit_byoc_job` against a modern signer → `type:byoc` + caps proto → 200 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
tests/test_media_publish.py (1)
730-730: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused unpacked
segment. Rename to_segmentto silence Ruff RUF059.♻️ Proposed tweak
- media, segment = self._build_drain_media(fail_after=1) + media, _segment = self._build_drain_media(fail_after=1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_media_publish.py` at line 730, In the test setup using _build_drain_media(fail_after=1), rename the unused unpacked segment variable from segment to _segment to satisfy Ruff RUF059 while preserving the media value and test behavior.Source: Linters/SAST tools
tests/test_stats_pull.py (2)
216-226: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClass-level
instancesregistries leak across tests. Only two tests clear them (Lines 564, 894) before assertinglen(instances) == 1; any future test that instantiates these fakes without clearing will break those assertions. An autouse fixture orpytest.fixturethat clears both lists would make this robust. Ruff also flags RUF012 here.Also applies to: 262-272
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_stats_pull.py` around lines 216 - 226, Update the _TrackingPacketDemuxer and corresponding fake class registries to avoid class-level mutable defaults flagged by RUF012, and add an autouse pytest fixture that clears both instances lists before each test. Remove reliance on individual test cleanup while preserving the existing length assertions.Source: Linters/SAST tools
467-474: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace hand-rolled module-attribute swaps with
mock.patch.object. Theoriginal_x = mod.X; mod.X = fake; try/finally: mod.X = original_xpattern is repeated ~10 times;with mock.patch.object(media_output_mod, "MpegTsDecoder", lambda: _FakeDecoder([...])):(or pytest'smonkeypatch) removes the boilerplate and the# type: ignorenoise, and is restore-safe by construction.Also applies to: 508-515, 532-540, 548-556, 565-572, 619-626, 661-668, 720-727, 895-902, 958-970, 1177-1192
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_stats_pull.py` around lines 467 - 474, Replace the manual save/assign/try-finally restoration pattern around MpegTsDecoder and the other repeated module-attribute swaps in the affected tests with mock.patch.object context managers (or pytest monkeypatch). Keep each test’s existing fake implementation and assertions unchanged while removing the original-value variables, explicit restoration, and associated type-ignore comments.tests/test_live_payment_session.py (1)
16-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClear the signer-info cache before the test too. Currently only teardown clears it, so a cached entry populated by another module (
get_signer_infois process-global) can maketest_get_signer_info_caches_resultobserve zero calls and fail.♻️ Proposed tweak
`@pytest.fixture`(autouse=True) def clear_signer_info_cache(self): + get_signer_info.cache_clear() yield get_signer_info.cache_clear()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_live_payment_session.py` around lines 16 - 19, Update the clear_signer_info_cache fixture to call get_signer_info.cache_clear() before yielding as well as during teardown, ensuring each test starts with an empty process-global signer-info cache while preserving cleanup afterward.tests/test_websocket_example.py (1)
13-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLoad the example module lazily so missing example deps skip instead of breaking collection.
spec.loader.exec_moduleruns at import time, so any optional dependency (e.g. a websocket client) or side effect inexamples/ping-pong/runner.pyturns into a collection error for the whole session rather than a skipped test.♻️ Proposed refactor
-spec = importlib.util.spec_from_file_location("websocket_runner_example", RUNNER_PATH) -assert spec is not None -runner = importlib.util.module_from_spec(spec) -assert spec.loader is not None -spec.loader.exec_module(runner) +@pytest.fixture(scope="module") +def runner(): + spec = importlib.util.spec_from_file_location( + "websocket_runner_example", RUNNER_PATH + ) + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + try: + spec.loader.exec_module(module) + except ImportError as exc: + pytest.skip(f"ping-pong example unavailable: {exc}") + return moduleThen take
runneras a test argument in both test methods.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_websocket_example.py` around lines 13 - 17, Move the example-module loading currently performed at test module scope into a pytest fixture, importing it lazily via spec.loader.exec_module and skipping when optional dependencies are unavailable. Update both test methods to accept the fixture-provided runner instead of relying on the global runner object, while preserving their existing assertions.tests/test_control_keepalive.py (1)
72-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPatching
control_mod.asyncio.sleepmutates the globalasynciomodule.control_mod.asynciois the stdlib module object, so this replacesasyncio.sleepprocess-wide for the duration of the block; any other coroutine that sleeps during that window will block onpermits. It works today because these tests are serial, but it is fragile under-p xdist/parallel loops. Prefer injecting the sleep function intoControl(or exposing a module-level_sleepalias incontrol.py) and patching that instead. Same pattern at Lines 106 and 131.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_control_keepalive.py` around lines 72 - 78, Stop patching control_mod.asyncio.sleep in the keepalive tests because it mutates the shared stdlib asyncio module; introduce a Control-level sleep dependency or module-level _sleep alias in control.py, use it from the keepalive implementation, and update the patches at the referenced test cases (including the occurrences around lines 106 and 131) to target that isolated symbol instead.pyproject.toml (1)
39-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSet
asyncio_default_fixture_loop_scopeexplicitly. With pytest-asyncio ≥0.24 inautomode, leaving this unset emits a deprecation warning when async fixtures use loop-scoped execution; setting it now keeps test output clean and future-proofs the event-loop scope.♻️ Proposed tweak
asyncio_mode = "auto" +asyncio_default_fixture_loop_scope = "function"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` around lines 39 - 46, Update the [tool.pytest.ini_options] configuration to set asyncio_default_fixture_loop_scope explicitly, choosing the intended fixture event-loop scope and keeping it consistent with the existing asyncio_mode = "auto" behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/tests.yml:
- Line 16: Update the actions/checkout step in the tests workflow to set
persist-credentials to false, ensuring the checkout token is unavailable to
subsequent pytest and test steps.
In `@tests/test_decode_metrics_sim.py`:
- Around line 1-8: Add the missing livepeer_gateway.decode_metrics_sim module
exposing _actual_decoder_snapshot and simulate_decoder_metric_drift so
tests/test_decode_metrics_sim.py imports successfully; implement the expected
simulation behavior used by the test, and make its timing or drift calculation
deterministic enough to preserve the max_abs_drift bounds without relying on
wall-clock scheduling.
In `@tests/test_multi_track_verify.py`:
- Line 6: Provide the missing livepeer_gateway.multi_track_verify module or
update the test import to the correct existing module. Ensure the module exposes
_goertzel_power, _verify_audio_track, _match_video_tracks, default_audio_specs,
default_video_specs, ObservedAudioTrack, ObservedVideoTrack, and
VideoFrameObservation so tests can collect without ImportError.
In `@tests/test_stats_pull.py`:
- Around line 1094-1109: Update
test_decoder_output_wait_metrics_accumulate_blocked_get_time to remove the tight
output_wait_s upper-bound assertion, retaining the >= 0.02 lower bound that
verifies blocked wait time accumulation.
In `@tests/test_trickle_shutdown_races.py`:
- Around line 154-155: Update the pytest.raises call around publisher.next() to
use a raw string for the regular-expression match pattern, preserving the
existing “closed|closing” alternatives and exception assertion.
---
Nitpick comments:
In `@pyproject.toml`:
- Around line 39-46: Update the [tool.pytest.ini_options] configuration to set
asyncio_default_fixture_loop_scope explicitly, choosing the intended fixture
event-loop scope and keeping it consistent with the existing asyncio_mode =
"auto" behavior.
In `@tests/test_control_keepalive.py`:
- Around line 72-78: Stop patching control_mod.asyncio.sleep in the keepalive
tests because it mutates the shared stdlib asyncio module; introduce a
Control-level sleep dependency or module-level _sleep alias in control.py, use
it from the keepalive implementation, and update the patches at the referenced
test cases (including the occurrences around lines 106 and 131) to target that
isolated symbol instead.
In `@tests/test_live_payment_session.py`:
- Around line 16-19: Update the clear_signer_info_cache fixture to call
get_signer_info.cache_clear() before yielding as well as during teardown,
ensuring each test starts with an empty process-global signer-info cache while
preserving cleanup afterward.
In `@tests/test_media_publish.py`:
- Line 730: In the test setup using _build_drain_media(fail_after=1), rename the
unused unpacked segment variable from segment to _segment to satisfy Ruff RUF059
while preserving the media value and test behavior.
In `@tests/test_stats_pull.py`:
- Around line 216-226: Update the _TrackingPacketDemuxer and corresponding fake
class registries to avoid class-level mutable defaults flagged by RUF012, and
add an autouse pytest fixture that clears both instances lists before each test.
Remove reliance on individual test cleanup while preserving the existing length
assertions.
- Around line 467-474: Replace the manual save/assign/try-finally restoration
pattern around MpegTsDecoder and the other repeated module-attribute swaps in
the affected tests with mock.patch.object context managers (or pytest
monkeypatch). Keep each test’s existing fake implementation and assertions
unchanged while removing the original-value variables, explicit restoration, and
associated type-ignore comments.
In `@tests/test_websocket_example.py`:
- Around line 13-17: Move the example-module loading currently performed at test
module scope into a pytest fixture, importing it lazily via
spec.loader.exec_module and skipping when optional dependencies are unavailable.
Update both test methods to accept the fixture-provided runner instead of
relying on the global runner object, while preserving their existing assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 73d9f912-6504-4b40-9eb7-3b78e6919415
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.github/workflows/tests.ymlREADMEpyproject.tomlsrc/livepeer_gateway/media_publish.pytests/test_byoc_training.pytests/test_channel_reader.pytests/test_control_keepalive.pytests/test_decode_metrics_sim.pytests/test_discovery.pytests/test_live_payment_session.pytests/test_live_runner.pytests/test_media_publish.pytests/test_multi_track_verify.pytests/test_selection.pytests/test_start_scope.pytests/test_stats_pull.pytests/test_token.pytests/test_trickle_shutdown_races.pytests/test_websocket_example.py
💤 Files with no reviewable changes (1)
- tests/test_byoc_training.py
| pytest: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v7 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow excerpt =="
if [ -f .github/workflows/tests.yml ]; then
cat -n .github/workflows/tests.yml
else
echo ".github/workflows/tests.yml not found"
fi
echo
echo "== checkout usages =="
rg -n "actions/checkout|persist-credentials|pytest|pull_request|permissions:" .github/workflows || true
echo
echo "== workflow files list =="
git ls-files .github/workflowsRepository: livepeer/livepeer-python-gateway
Length of output: 1465
🌐 Web query:
actions/checkout default persist-credentials true README
💡 Result:
In the actions/checkout GitHub Action, the persist-credentials input controls whether the authentication token or SSH key is configured in the local git configuration [1][2][3]. When set to true (which remains the default), the credentials are made available to subsequent steps in the job to allow authenticated git operations [1][2]. The action automatically removes these credentials during the post-job cleanup phase [1][2]. Starting with version 6.0.0, the action improved security by storing these credentials in a separate file under the $RUNNER_TEMP directory instead of writing them directly into the.git/config file [2][4]. While there has been community discussion and proposals to change the default value of persist-credentials to false to minimize security risks [5][6], the default value remains true in the current official documentation and action configuration [2][3]. Users who wish to disable this behavior and prevent credentials from being available to other steps in their workflow can explicitly set persist-credentials: false [1][2].
Citations:
- 1: https://github.com/marketplace/actions/checkout?version=v3.6.0
- 2: https://github.com/actions/checkout/tree/v6.0.2?tab=readme-ov-file
- 3: https://github.com/actions/checkout/blob/72f2cec99f417b1a1c5e2e88945068983b7965f9/action.yml
- 4: https://github.com/actions/checkout/tree/v6.0.0
- 5: Change the default value of persist-credentials to false actions/checkout#1687
- 6: Remove
persist-credentialsor change the default tofalseactions/checkout#485
Disable checkout credential persistence
This workflow runs pull-request code via pytest and does not need authenticated Git operations after checkout. Set persist-credentials: false so the checkout token is not made available to later test steps.
Proposed fix
- - uses: actions/checkout@v7
+ - uses: actions/checkout@v7
+ with:
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/checkout@v7 | |
| - uses: actions/checkout@v7 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 16-16: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/tests.yml at line 16, Update the actions/checkout step in
the tests workflow to set persist-credentials to false, ensuring the checkout
token is unavailable to subsequent pytest and test steps.
Source: Linters/SAST tools
| import math | ||
| from array import array | ||
|
|
||
| from livepeer_gateway import multi_track_verify as verify_mod |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -i 'multi_track_verify*' .
rg -n 'multi_track_verify' --glob '!tests/test_multi_track_verify.py'
rg -n '_goertzel_power|default_audio_specs|ObservedAudioTrack|_match_video_tracks' -g '!tests/**'Repository: livepeer/livepeer-python-gateway
Length of output: 205
🏁 Script executed:
#!/bin/bash
set -e
echo "== file listing =="
git ls-files | sed -n '1,200p'
echo "== test import and referenced symbols =="
cat -n tests/test_multi_track_verify.py
echo "== package files and livepeer_gateway references =="
fd -i 'livepeer_gateway' . || true
rg -n 'livepeer_gateway|multi_track_verify|package|packages' pyproject.toml setup.py setup.cfg poetry.lock uv.lock Pipfile requirements*.txt tests/test_multi_track_verify.py || trueRepository: livepeer/livepeer-python-gateway
Length of output: 50389
Add the missing multi_track_verify package module. tests/test_multi_track_verify.py imports livepeer_gateway.multi_track_verify, but only the test file exists; no matching module or referenced helper symbols are present, so collection fails with ImportError. Add src/livepeer_gateway/multi_track_verify.py with _goertzel_power, _verify_audio_track, _match_video_tracks, default_audio_specs, default_video_specs, ObservedAudioTrack, ObservedVideoTrack, and VideoFrameObservation, or point the import at the correct package module.
🧰 Tools
🪛 GitHub Actions: Tests / 0_pytest.txt
[error] 6-6: Pytest import error during test collection: ImportError: cannot import name 'multi_track_verify' from 'livepeer_gateway' ('/home/runner/work/livepeer-python-gateway/livepeer-python-gateway/src/livepeer_gateway/init.py').
🪛 GitHub Actions: Tests / pytest
[error] 6-6: Pytest failed during test collection due to import error: ImportError: cannot import name 'multi_track_verify' from 'livepeer_gateway' (/home/runner/work/livepeer-python-gateway/livepeer-python-gateway/src/livepeer_gateway/init.py).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_multi_track_verify.py` at line 6, Provide the missing
livepeer_gateway.multi_track_verify module or update the test import to the
correct existing module. Ensure the module exposes _goertzel_power,
_verify_audio_track, _match_video_tracks, default_audio_specs,
default_video_specs, ObservedAudioTrack, ObservedVideoTrack, and
VideoFrameObservation so tests can collect without ImportError.
Source: Pipeline failures
| def test_decoder_output_wait_metrics_accumulate_blocked_get_time(self) -> None: | ||
| decoder = MpegTsDecoder() | ||
|
|
||
| def _put_later() -> None: | ||
| time.sleep(0.03) | ||
| decoder._put_output_item(object()) | ||
|
|
||
| producer = threading.Thread(target=_put_later, daemon=True) | ||
| producer.start() | ||
| got = decoder.get() | ||
| producer.join() | ||
|
|
||
| assert got is not None | ||
| stats = decoder.get_stats() | ||
| assert stats.output_wait_s >= 0.02 | ||
| assert stats.output_wait_s < 0.25 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Wall-clock assertion may be flaky under load. output_wait_s < 0.25 after a 0.03s sleep is tight for a loaded CI runner where the producer thread can be descheduled. Consider dropping the upper bound (or widening it substantially) and keeping only the >= 0.02 lower bound, which is what the metric actually needs to prove.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_stats_pull.py` around lines 1094 - 1109, Update
test_decoder_output_wait_metrics_accumulate_blocked_get_time to remove the tight
output_wait_s upper-bound assertion, retaining the >= 0.02 lower bound that
verifies blocked wait time accumulation.
| with pytest.raises(RuntimeError, match="closed|closing"): | ||
| await publisher.next() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a raw string for the regex. Ruff (RUF043) flags the unescaped alternation; the intent is a regex, so mark it raw.
🩹 Proposed fix
- with pytest.raises(RuntimeError, match="closed|closing"):
+ with pytest.raises(RuntimeError, match=r"closed|closing"):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with pytest.raises(RuntimeError, match="closed|closing"): | |
| await publisher.next() | |
| with pytest.raises(RuntimeError, match=r"closed|closing"): | |
| await publisher.next() |
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 154-154: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_trickle_shutdown_races.py` around lines 154 - 155, Update the
pytest.raises call around publisher.next() to use a raw string for the
regular-expression match pattern, preserving the existing “closed|closing”
alternatives and exception assertion.
Source: Linters/SAST tools
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_decoder_queue_metrics.py (1)
149-167: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace
getattrwith direct attribute access.
getattr(decoder, "_reader"),getattr(reader, "_queue"),getattr(decoder, "_output"), andgetattr(reader, "_buffer")use a constant attribute name. Direct attribute access is equivalent and clearer. Ruff flags all four calls (B009).♻️ Proposed refactor
def _actual_decoder_snapshot(decoder: object) -> tuple[int, int, int, int]: - reader = getattr(decoder, "_reader") - input_queue = getattr(reader, "_queue") + reader = decoder._reader + input_queue = reader._queue with input_queue.mutex: input_items = list(input_queue.queue) queued_payloads = [ item for item in input_items if isinstance(item, (bytes, bytearray, memoryview)) ] - output_queue = getattr(decoder, "_output") + output_queue = decoder._output with output_queue.mutex: output_items_queued = len(output_queue.queue) return ( len(queued_payloads), sum(len(item) for item in queued_payloads), - len(getattr(reader, "_buffer")), + len(reader._buffer), output_items_queued, )As per static analysis hints, Ruff flags
getattrwith a constant attribute value at Lines 150, 151, 159, and 165 (B009).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_decoder_queue_metrics.py` around lines 149 - 167, Update _actual_decoder_snapshot to replace the four constant-name getattr calls with direct attribute access on decoder and reader: _reader, _queue, _output, and _buffer. Preserve the existing queue locking, filtering, and snapshot calculations.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_decoder_queue_metrics.py`:
- Around line 149-167: Update _actual_decoder_snapshot to replace the four
constant-name getattr calls with direct attribute access on decoder and reader:
_reader, _queue, _output, and _buffer. Preserve the existing queue locking,
filtering, and snapshot calculations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c640a1e8-2c98-446e-b181-efa7cdea3c5d
📒 Files selected for processing (3)
tests/test_decoder_queue_metrics.pytests/test_live_payment_session.pytests/test_media_publish.py
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Content-type detection was a substring test (`"json" in content_type`), which is right for `+json` types only by accident and wrong for multi-document formats: `application/jsonl` (this repo's own trickle channel default), `application/x-ndjson`, and `application/json-seq` all matched and then failed in json.loads, so a working runner fronting a streaming API got "did not return valid JSON" instead of its bytes — the same bug class this branch set out to fix. Match on the media subtype instead, via aiohttp's own mimetype parser: `application/json` or the RFC 6839 `+json` structured suffix. Vendor types (`application/vnd.acme.v1+json`) keep parsing without being listed; multi-document bodies fall through to `raw`. Only four classifications change, all previously raising. Also fold the raw early return into the single existing return, so `payment_session=None if payment_type == "fixed" else payment_session` stays in one place — payment-type handling here has been reverted twice and there is no paid-binary test to catch the two sites drifting. The `session_id` expression reduces to the early return's behavior when `data` is empty, so this is equivalent. Add `content_type` to the JSON parse error: it is the fact that routed the response into parsing, and without it a misclassification is undebuggable from the message alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`content_type` is self-documenting from the field name; the invariant worth stating is that a populated `raw` means an empty `data`. The subtype helper matches its bare neighbors in this module. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`raw` already means "the original JSON dict, un-normalized" on three dataclasses in this SDK — LiveRunnerSessionEvent.raw, LiveRunnerInstance.raw, LiveVideoToVideo.raw — and examples dump it with json.dumps(x.raw). A `raw: Optional[bytes]` on LiveRunnerCallResult overloads the name to mean the opposite, sharply so on one expression chain: `result.raw` (bytes) sitting one dot from `result.runner.raw` (dict). `content` matches its sibling `content_type` and the ecosystem convention for a response body as bytes (requests/httpx `.content`) — where `.raw` instead means an unread stream object, so the old name actively misled. Nothing consumes the field yet (runner-app-examples#45 and api-proxy both use the streaming path), so this is free now and permanent once released. Semantics unchanged: still `Optional[bytes] = None`, since `b""` is a valid empty body and `None` is the only unambiguous "this was JSON" sentinel. Also renames the local `raw` to `body` — it holds bytes, while `raw` in this codebase reads as a dict. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
One helper builds every SDK request and hardcodes Accept: application/json. That is right for the control plane, where the orchestrator is a JSON API, but call_runner is the one function whose destination is arbitrary app code. An upstream that content negotiates then answers in JSON because the SDK asked: the api-proxy example advertises raw JPEG bytes and Hugging Face returned a base64 PNG in a JSON string, so the client wrote base64 text to a .jpg. App calls now state no preference and let the app pick. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
apologies for the force-push, I didn't realize updating GitHub stacked PRs would also change the base branch ... ugh |
| if not task.done(): | ||
| task.cancel() | ||
| with contextlib.suppress(asyncio.CancelledError): | ||
| await task |
3 participants
Summary by CodeRabbit