Studio CI: make tool-calling SSE probes resilient to transport stalls - #7137
Conversation
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b1055ca57
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if events: | ||
| print(f"[retry-sse] {path}: {exc!r}; keeping {len(events)} partial events", flush = True) | ||
| return "".join(parts), events |
There was a problem hiding this comment.
When a Studio tool stream emits only an early tool_start/delta.tool_calls event and then stalls before tool_end or the final assistant text, this path returns those partial events instead of retrying or raising. In this workflow, _tool_invoked() treats that event as success evidence and _run_tool_probe() later returns a WARN if any attempt was invoked, so a broken or hung server-side tool loop can make the Linux smoke job pass as long as one partial invocation event was seen.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0ff9088. The Linux post_sse no longer returns partial events on a stall: a stalled stream is incomplete, so an early tool_start with no tool_end can no longer satisfy _tool_invoked and WARN-pass a hung tool loop. A stall now raises and _run_tool_probe rotates to the next seed (or fails if every seed stalls). Windows and mac gate on the literal answer in content, so their partial-content return stays safe.
| "max_tokens": 600, | ||
| }) | ||
| if "56088" in content or "56,088" in content: | ||
| }, soft = True) |
There was a problem hiding this comment.
In the Windows tool-calling job, this soft probe still uses post_sse's default timeout=600,retries=1, so a no-data stall waits up to 600s + 15s + 300s before returning the intended non-blocking WARN. The next terminal soft probe can then do the same, and with the job capped at 30 minutes this can be killed by the workflow timeout before the thinking checks run or logs finish normally; pass a shorter timeout as the Mac workflow does, or stop after the first soft stall.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0ff9088. Both Windows soft probes now pass timeout=180, retries=0, so a no-data stall is a single 180s attempt instead of 600+15+300s. Worst case for the two probes drops from about 30.5 min to about 6 min, well inside the 30-min job cap, leaving room for the thinking checks. The web_search probe is bounded the same way.
| "temperature": TOOL_PROBE_TEMP, | ||
| "seed": attempt_seed, | ||
| "max_tokens": 600, | ||
| }) |
There was a problem hiding this comment.
In the Linux tool-calling job (timeout-minutes: 25), a no-data stall on this call now spends post_sse's default 600s, sleeps 15s, then spends the 300s retry before the catch can rotate to the next seed. Since _run_tool_probe allows four seeds and runs for both python and terminal, the workflow can be killed by its timeout before producing the intended assertion or reaching later checks; pass a shorter per-attempt timeout or cap the total seed-rotation budget.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0ff9088. The Linux _run_tool_probe now calls post_sse with timeout=180, retries=0 (the seed loop is the retry) and caps total seed rotation with a 300s wall-clock budget, so a persistent stall fails fast with a clean assertion instead of the 4x915s that could hit the 25-min job timeout. A healthy or degenerate round answers in seconds, so all four seeds still run in the normal case.
…cepting partial tool events
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ff9088ea0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| time.sleep(15) | ||
|
|
||
| def post_sse(path, body, *, timeout = 600): | ||
| def post_sse(path, body, *, timeout = 600, retries = 1, soft = False): |
There was a problem hiding this comment.
In the macOS tool-calling job, this new default makes both best-effort post_sse(..., timeout=180) probes retry for another 180s after a no-data stall, so the python probe plus the web_search probe can spend 12.5 minutes before merely warning. Since this job has a 25-minute timeout and model load alone is allowed to take up to 10 minutes, the intended non-blocking stall path can still fail the workflow by timing out before the thinking checks run; mirror the Windows calls by disabling retries for these best-effort probes or use a smaller bound.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0daaecf. Both Mac best-effort probes (the soft python probe and web_search) now pass retries=0, matching the Windows calls, so a no-data stall is a single 180s attempt instead of 180+15+180s. The two probes drop from about 12.5 min worst case to about 6 min, keeping the 25-min job under budget even with a 10-min model load, so the non-blocking stall path no longer risks a workflow timeout before the thinking checks.
| "seed": attempt_seed, | ||
| "max_tokens": 600, | ||
| }, timeout = 180, retries = 0) | ||
| except (TimeoutError, ConnectionError, urllib.error.URLError) as exc: |
There was a problem hiding this comment.
When a server-side tool probe in this workflow gets an HTTP 4xx/5xx from /v1/chat/completions, post_sse correctly re-raises HTTPError, but HTTPError is a subclass of urllib.error.URLError, so this new handler catches it as a transport stall and rotates seeds instead of surfacing the status immediately. That can hide a transient endpoint failure if a later seed passes, and even persistent status failures are reported as missing tool-invocation evidence rather than the real HTTP regression; exclude HTTPError here or re-raise it before treating URLError as retryable transport.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0daaecf. _run_tool_probe now catches urllib.error.HTTPError and re-raises it before the transport-stall handler. Since HTTPError subclasses URLError it was being swallowed as a stall and rotating seeds; now a real 4xx/5xx from /v1/chat/completions surfaces immediately instead of being masked as missing tool-invocation evidence. Verified by exercising the extracted _run_tool_probe with real urllib.error classes: an HTTPError propagates while a TimeoutError still rotates all seeds.
…est-effort probes
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0daaecf3cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if attempt == retries: | ||
| raise |
There was a problem hiding this comment.
When the Ubuntu tool probe hits a read timeout after Studio has already streamed the useful tool events, this branch discards the accumulated events and _run_tool_probe records only a transport error. In the exact stall-after-answer case this change is meant to tolerate, repeated seeds can still fail with “no structural tool-invocation evidence” even though a tool_end/expected result was already received; consider returning or evaluating partial events once they include completed tool output rather than always raising here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9b5a023. post_sse now takes a complete_on predicate; on a stall it returns the events already collected only when they satisfy it. _run_tool_probe passes complete_on = _tool_invoked and _tool_output_contains(needles), so a stall that arrives after the tool_end result was received keeps those events and the probe passes, while a stall with only an early tool_start (no completed output) still raises and rotates seeds. That preserves the earlier guarantee (a tool_start-only hung loop cannot pass) and restores the stall-after-answer tolerance.
| if attempt_i and time.monotonic() > probe_deadline: | ||
| print(f"[tools] {label}: seed-rotation budget spent after {attempt_i} attempts", flush = True) | ||
| break |
There was a problem hiding this comment.
In the Ubuntu tool-calling job, this guard only checks whether the 300s seed-rotation budget has already expired before starting the next request. If attempt 0 stalls for the full 180s, attempt 1 still starts with about 120s left and can block for another 180s, so each _run_tool_probe can take ~360s; because the workflow runs both python and terminal probes under a 25-minute job cap, persistent no-data stalls can still consume the job timeout before the later hard checks run. Cap the next post_sse timeout by the remaining budget or stop before launching an attempt that cannot finish inside it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9b5a023. The loop now caps each read by the remaining budget (timeout = min(180, remaining)) and skips an attempt with under 30s left, so the whole seed rotation stays within the ~300s budget instead of the previous 2x180s. With two probes under the 25-minute job cap, a persistent no-data stall can no longer reach the job timeout before the hard checks run.
…by the remaining budget
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Problem
The "Tool calling Tests" job in the Studio inference-smoke workflows boots a real Studio server on a shared CI runner and drives the server-side tool loop over SSE (
/v1/chat/completionswithenable_tools). On the free Windows and macOS runners, CPU inference of the Qwen3.5-2B GGUF is slow, and the SSE stream can stall at the transport level (the connection opening, or a mid-stream read) even when Studio is healthy.When that happens,
post_sseraisesTimeoutError: timed outfromsocket.recv_intoand the whole job fails, even though the meaningful tool-call contract (the non-streaming function-calling probe, and the thinking on/off probe) already passed. A recent example failed exactly this way: theget_weatherfunction-calling probe passed, then the python-tool SSE probe stalled with no tokens and the job exited 1.Change
Harden the SSE read in all three inference-smoke workflows without weakening the real assertions:
post_ssenow retries a transport stall once with a fresh request capped at 300s (mirroring the existingposthelper), and returns any text already streamed before a stall, so a stall on the trailing tokens after the answer arrived still counts.soft: a total transport stall that yields nothing becomes a non-blocking WARN instead of failing the job, the same philosophy already applied to theweb_searchprobe.studio-inference-smoke.ymlthe multi-seed_run_tool_probenow also catches a transport stall and rotates to the next seed rather than propagating it, and itspost_ssegains the same retry plus partial-return.Files
.github/workflows/studio-windows-inference-smoke.yml.github/workflows/studio-mac-inference-smoke.yml.github/workflows/studio-inference-smoke.ymlValidation
Extracted the real
post_ssetext from each workflow and exercised it against a mocked stream: a clean stream returns full content; a mid-stream stall returns the partial content already received; a total stall returnsNoneundersoft(or re-raises for seed rotation in the generic file) after using both attempts; and an HTTP status error surfaces immediately without retry. All three embedded Python heredocs compile and the YAML parses.