Trim and tighten code comments and docstrings across the repository. Comment-only: every changed file verified code-identical to main via AST/token comparison.
Trim and tighten code comments and docstrings across studio/ Python. Comment-only: every changed file verified code-identical to main via AST/token comparison.
Raise ruff line-length to 100 and extend the local pre-commit format pipeline (def-signature magic-comma normalization, short multi-line assert collapse, kwarg '=' spacing, blank-line-after-short-import removal, adjacent string-literal / f-string+plain merge, redundant-pass pruning). Every transform re-checks the file AST and is dropped if it would differ; the whole-repo reformat is verified AST-identical per file and idempotent.
* studio: scope cancel-cleanup to in-flight tmp dirs; walk back tool_call_id
Two follow-ups to #5375's training and chat hardening.
_cleanup_cancelled_checkpoints used to rmtree every checkpoint-N
directory on Cancel. That is the opposite of what the user expects.
A user cancelling an 8h run with save_steps=2000 loses every
completed checkpoint they could have resumed from. The 67 MB residue
the audit memo flagged is the HF Trainer atomic-rename partial
(tmp-checkpoint-N), not the completed ones. The cleanup now targets
only tmp-checkpoint subdirs; completed checkpoint-N directories are
user-owned and stay. Symlinked output_dir and symlinked children are
skipped so the realpath containment cannot be levered into deleting
arbitrary content via a symlink trick.
ChatMessage._validate_role_shape stamped a random secrets.token_hex
id on tool messages with no tool_call_id. That id is uncorrelated
with the prior assistant tool_calls id, so strict passthrough
backends (OpenAI, Anthropic) reject the request as orphaned and
llama.cpp treats the tool result as "no preceding call" and
hallucinates. The synthesis moves up to ChatCompletionRequest, where
the whole conversation is visible: for each tool message missing an
id we walk back to the most recent assistant turn with tool_calls
(stopping at user turns), prefer a function.name match, otherwise
take the first unconsumed tool_call. Synthesis is the fallback when
no candidate assistant turn exists, preserving the prior round-trip
guarantee for orphaned tool messages.
Tests:
- test_cleanup_cancelled_checkpoints.py (new): pins that completed
checkpoint subdirs survive, tmp-checkpoint partials are removed,
non-int suffixes (checkpoint-final, checkpoint-best) are left
alone, output_dir outside outputs_root is refused, symlinked
output_dir and symlinked child are both skipped, missing dir is
a no-op.
- test_inference_model_validation.py: 6 new walkback cases covering
name-match preference, first-unconsumed fallback, explicit-id
passthrough, multi-tool-result pairing, synth-on-no-parent, and
no-cross-user-turn invariant.
- test_openai_tool_passthrough.py: the two ChatMessage-level
synth-on-missing tests are rewritten to assert that the per-
message validator now leaves tool_call_id untouched; resolution
coverage lives in the request-level tests above.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* studio: explicit tool_call_id reserve, numeric tmp-checkpoint suffix only
Reviewer follow-ups to the training-cleanup + tool_call_id walkback PR.
tool_call_id walkback: a mixed assistant turn with [call_a, call_b]
followed by a tool result that carried tool_call_id="call_a" and a
sibling tool result with no id resolved to ['call_a', 'call_a']
because the explicit id never reserved call_a in the consumed set.
Added a pre-pass over the message list that walks back from every
role="tool" message carrying an explicit id and marks the matching
(asst_idx, tc_idx) consumed, then the missing-id walkback runs against
that pre-populated set. The second result now resolves to call_b.
While here, also harden the function-shape check: if a provider
ships a malformed tool_call where `function` is a string rather than
a dict, the old `(tc.get("function") or {}).get("name")` raised
AttributeError on the string's .get; now isinstance-gated so the
walkback falls through to the fallback id without raising.
Cancel cleanup: `tmp-checkpoint-*` is too broad. HF Trainer's
in-flight partials are always `tmp-checkpoint-<integer-step>`, so
constrain the cleanup regex to `^tmp-checkpoint-\d+$`. A user folder
named `tmp-checkpoint-final`, `tmp-checkpoint-backup`, or
`tmp-checkpoint-user-notes` is now preserved.
ChatMessage docstring still pointed at the pre-PR contract that
required `tool_call_id` on every role="tool" message. Updated to say
missing ids are accepted at message scope and resolved at
ChatCompletionRequest scope. Inline comment above the cancel-cleanup
call now describes the actual behaviour (in-flight tmp partials,
completed checkpoints preserved).
Test:
- python -m pytest studio/backend/tests/test_inference_model_validation.py
studio/backend/tests/test_cleanup_cancelled_checkpoints.py
studio/backend/tests/test_openai_tool_passthrough.py -q
-> 76 passed (was 67 before this commit; +2 walkback regression
tests, +1 numeric-suffix preservation test)
* studio: trim verbose comments in cleanup + tool_call_id walkback
Move the HF tmp-checkpoint regex to module scope as a named constant.
Drop the multi-paragraph docstring on _cleanup_cancelled_checkpoints
and the inline call-site rationale; the function name + the test
class already cover the why.
Compress _resolve_missing_tool_call_ids docstring from a six-line
explanation to two. Same logic, fewer in-flow tutorials.
76 tests in cleanup + inference-model-validation + tool-passthrough pass.
---------
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
