Repository navigation
Add CI, lint and pre-commit, and repair the full test suite - #166
Merged
ProfSynapse merged 23 commits intoOct 2, 2026
Merged
Conversation
Add [tool.ruff] to pyproject.toml selecting only the F family, excluding Datasets/, scratch/, notebooks and the generated .agents/.claude skill mirrors. No formatter or other rule family is enabled. Fixes that change behaviour (found via F821/F811/F402): - Trainers/sft/train_sft.py: the post-training loss hook referenced an undefined _REPO_ROOT, so a relative train_dataset raised NameError that the surrounding except swallowed and per-example losses were silently skipped. Resolve against Path(__file__).parent.parent.parent, the repo-root expression the module already uses for its sys.path bootstrap, with a source-level regression test. - Evaluator/ui.py: the "... and N more failures" footer sat unreachable after a return in _expected_for_display; move it back to the end of rich_failure_details. - tests/cloud/test_cloud_eval_handler.py: _canonical_repo_source() fell off the end returning None; its return statement had been displaced after _source_preparation()'s return. - tests/execution/providers/test_modal_packaged_dispatch.py: drop a byte-identical duplicate of three tests/helpers and duplicate imports. - Redundant local re-imports (SynthChat engine/renderer), loop variables shadowing dataclasses.field, an unused field import. Mechanical: unused imports/variables (F401/F841, reviewed; calls with side effects kept as bare calls), f-strings without placeholders (F541), and star imports in tests replaced by explicit imports. Availability probes, registration imports, pytest fixture imports, re-exported script helpers and a monkeypatch seam keep their imports with a targeted noqa and reason. Hashed source inventories: Trainers/sft/train_sft.py and Trainers/sft/runtime_v1.py are fixed and the offline SFT worker closure and Modal runtime lock were refreshed with their checked-in scripts (hash-only changes; inference lock and packaged-worker closure untouched, all four checks CURRENT). The remaining locked members are not edited; their findings stay in per-file-ignores until the next deliberate refresh that covers them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013etFNjLuo22CCoQmUGYqCT
.github/workflows/ci.yml (the path-filtered conformance workflow is unchanged): - lint: ruff check with the pyproject.toml configuration (F rules only). - tests: the whole pytest suite on ubuntu-24.04 / CPython 3.12, CPU-only torch, in three shards whose union is every test file. The Modal inference provider tests take most of the runtime and are split alphabetically into two shards; "core" runs everything else. A gate job requires all shards. Shard selection lives in scripts/ci_pytest_shard.sh so it runs identically locally. - Triggers on pull requests and pushes to main and feat/submodule-cloud-api-v1; superseded runs are cancelled, every job has a timeout, pip downloads are cached and JUnit XML is uploaded. requirements-ci-tests.txt holds the CPU test environment. GPU stacks (unsloth, vllm, bitsandbytes, mlx) and the modal SDK are left out; tests needing them skip themselves. tests/trainers/sft/__init__.py: without it the full suite stops at collection because tests/trainers/sft/test_runtime_v1.py and tests/ingestion/test_runtime_v1.py share a module basename. Sibling tests/trainers/* packages already have __init__.py. .pre-commit-config.yaml: ruff-check, check-yaml (multi-document allowed), check-toml, check-merge-conflict, check-added-large-files (2048 KB, tracked Datasets/ artifacts excluded) and actionlint. No hook rewrites files. check-yaml skips Datasets/ (three invalid legacy rubric drafts) and SynthChat/scenarios/destructive.yaml, which repeats storageManager_archive with different weights inside single tools maps and needs an owner fix. The suite is not green yet. Measured locally with these shard commands: core shard on this base: 11366 passed, 84 failed, 43 skipped, 2 collection errors; both Modal inference shards (740 tests) passed on the previous base. Failures are stale tests after config-first refactors and stricter config validation, order-dependent tests (Trainers/*/src/data_loader module-name collisions), Windows-only simulations, a huggingface_hub>=1 requirement that conflicts with transformers<5, and a few local-environment artifacts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013etFNjLuo22CCoQmUGYqCT
…split defaults TestTrainingRunnerHappyPath (and the other HFTrainingStageRunner.run tests that use _mock_backend) failed with "dataset.validation_group_key requires dataset.split_dataset: true" after the grouped-split work started validating split settings in the HF training stage. The validation is correct: real configs come from HFJobsBackend.load_config, where split_dataset / test_size / validation_group_key default to None, and validation_split_flags returns [] for that. The mock config was a bare MagicMock, so those attributes read back as truthy, non-None MagicMock children: a "set" group key without split_dataset: true. The test double now models the documented defaults instead of inventing an invalid combination. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013etFNjLuo22CCoQmUGYqCT
Trainers/{sft,dpo,grpo,kto,embedding,...}/src all ship a data_loader.py, and
the tests imported them the way the trainer entrypoints do: put src on
sys.path, then a bare "import data_loader". In one pytest process the first
trainer collected wins sys.modules["data_loader"], so the SFT data-loader,
label-fix and mask-doctor tests (and the GRPO chat-template tests) ran against
Trainers/dpo/src/data_loader.py and failed or errored, while passing alone.
tests/trainers/_trainer_import.py loads Trainers/<method>/src/<name>.py as
trainer_src_<method>_<name>. While the file executes, its bare sibling
imports (from data_loader import ..., from preprocessing import ...) resolve
only to the same trainer's src files, each also under its unique name; the
bare sys.modules entries are restored afterwards and sys.path is untouched.
Trainer runtime imports are unchanged.
The GRPO pivot-profiler test also installed a MagicMock as sys.modules["torch"]
whenever torch had not been imported yet (even when installed) and never
removed it, which broke every later datasets-based test. The helper now
stubs a module only when it is not installed, only while the file loads.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013etFNjLuo22CCoQmUGYqCT
test_coordinator_import_is_provider_sdk_and_storage_neutral asserted that no huggingface_hub/modal/runpod module was in this pytest process's sys.modules, which fails whenever any earlier test (e.g. anything importing datasets) has loaded huggingface_hub. Import the coordinator model, ports and state machine in a subprocess and assert on that interpreter's modules, which is what the test means. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013etFNjLuo22CCoQmUGYqCT
…rd error The check averaged 100 drop-and-rescale trials and required every element to be within an absolute 0.5 of the original. The per-element error scales with |x| * sqrt(p / (1 - p) / N), so large randn weights exceeded 0.5 by chance: about 6% of RNG states failed (measured over 3000 replicates), which made the outcome depend on how much global RNG earlier tests consumed. Use 1000 trials and a six-sigma bound on that standard error per element. False failures drop to ~1e-6 for the tensor, and the bound is below the old 0.5 for every |x| < 4, so it is stricter in practice (it also catches a 5% rescale bias the old check never caught). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013etFNjLuo22CCoQmUGYqCT
test_cloud_stage_event_appends_are_serialized_across_processes raised queue.Empty in the full run: each of 16 spawned children imported the whole experiment-handler test module (torch and the tracking stack) to find its target, and the parent gave each result a fixed 40 s. The target now lives in tests/cloud/_stage_event_worker.py, so a child imports only that and shared.cloud_stage_logging (the test drops from ~16 s to ~2 s here). The parent waits for the condition itself, every child reporting or every child gone with nothing left to read, under a 300 s bound, and the assertions report which children never answered. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013etFNjLuo22CCoQmUGYqCT
sync_bucket_prefix.py runs inside the warm vLLM Space with its own huggingface_hub>=1.0 venv (/opt/bucket-sync-venv, see Dockerfile.tmpl), because Buckets need a newer Hub client than the transformers 4.x stack allows. Its module-level `from huggingface_hub import sync_bucket` made the pure parse_bucket_uri helper unimportable next to the engine's pinned huggingface-hub<1.0, which broke collection of tests/cloud/test_manage_space.py. manage_space.py itself only uses APIs present in hub 0.x. Defer the import into sync_bucket_prefix(), the only code that needs it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013etFNjLuo22CCoQmUGYqCT
Past the capability gate, launch.execute() imports modal before it reaches
local storage. Use pytest.importorskip("modal") like the other Modal chat
example tests, since CI deliberately installs no provider SDKs.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013etFNjLuo22CCoQmUGYqCT
- docker_archive: the exclusive-output, byte-bound and stuck-reader tests are cross-platform; only the process-group mechanism differs. Assert the platform's own group kwargs (CREATE_NEW_PROCESS_GROUP / start_new_session) and stub the platform's tree stop. On POSIX the fake pid previously reached a real os.killpg(os.getpgid(4321), SIGKILL). - image_lock: assert platform group kwargs in the subprocess runner test. The two taskkill.exe tests are Windows-only behaviour and are marked os.name != "nt" like the repo's other Windows-only tests; the fallback one had been passing on Linux only by sending SIGKILL to pid 1234's real process group. Add POSIX counterparts that stub getpgid/killpg. - registry: the replace retry is deliberately gated on os.name == "nt" (POSIX os.replace has no sharing violations) and register_run locks with msvcrt vs fcntl by os.name, so the two retry tests are Windows-only. - snapshot_nexus: fake Windows for the module under test only. Patching the global os.name made pathlib build WindowsPath objects on POSIX. The envelope and batch-failure tests also stub executable resolution, as the argv test already does; they stubbed subprocess.run but still needed a real `nexus` CLI on PATH, so they failed with NEXUS_RUNTIME_UNAVAILABLE. - derived_training_image: set the module's _WINDOWS flag, as sibling tests do, for the two tests that exercise the Docker Desktop Buildx authority. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013etFNjLuo22CCoQmUGYqCT
SurrogateModel.feature_importance() is declared Dict[str, float], but LightGBM's default "split" importance type yields integer split counts, so the method returned ints and TestSurrogateModel::test_feature_importance failed wherever lightgbm is installed. Convert each importance to float. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013etFNjLuo22CCoQmUGYqCT
- token_capture: 0cbc018 deliberately replaced the {conversations, reward, token ids} GRPO row with the static-GRPO trainer schema and made test_stager assert it exactly, but left these tests on the old shape. Assert the current contract: captured token fields stay addressable via the filter view's content.* namespace and are not part of the exact row. - handlers versions: 8e58851 moved `flywheel versions` from repo_root (now a compatibility shim) to engine_root; patch engine_root so the fixture datasets directory is actually consulted. - hf_smoke parent-chain swap: c10120f replaced the re-run link check with held, handle-relative O_NOFOLLOW opens. A swapped parent still fails closed, now as "could not be read safely" (POSIX ELOOP) or "reparse points" (Windows); assert those instead of the retired message. - tracking provenance: restore the contained URI before the symlink case; the test still pointed at tracking://../outside.json, so the escape check fired first and the symlink refusal was never exercised. - cloud_artifacts non-git: bound git discovery with GIT_CEILING_DIRECTORIES so a basetemp under the repository's scratch/ cannot supply a repo. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013etFNjLuo22CCoQmUGYqCT
sync_file_to_hf_bucket called HfApi.upload_file with repo_type="bucket",
which huggingface_hub rejects ("Invalid repo type"), so every single-file
bucket push failed. Use batch_bucket_files, the bucket API the provider
adapter already uses, and name the qualified hub version when the installed
hub lacks it. Refresh the offline SFT worker closure and Modal runtime lock
for the one changed member.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013etFNjLuo22CCoQmUGYqCT
A tool rename mapped vaultManager_deleteFolder and vaultManager_deleteNote onto the single storageManager_archive tool, leaving each tools map with a repeated key (YAML kept the last weight). Collapse each pair to one entry, fix the matching prompt text, and stop excluding the file from check-yaml. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013etFNjLuo22CCoQmUGYqCT
Since 6dcb543 the built-in tool-call format has no wrapper; useTools is only the configured default in SynthChat/config/tool_call_formats.yaml. The available_tools section still always rendered "Use the `<wrapper>` wrapper for tool calls.", producing "Use the `` wrapper" for native formats such as openai_native. Emit that line only when a wrapper is configured. The workspace tests still expected useTools as the code default. They now assert that the built-in default has no wrapper, that the configured default format supplies its wrapper, and that the rendered section carries the configured wrapper line only when one exists. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013etFNjLuo22CCoQmUGYqCT
f5e9126 rewrote the vault gym environment allowlist as CLI command strings ("content read", "search content", ...). The environment executor expands each wrapped CLI command into its concrete tool (contentManager_read, searchManager_content, ...) before checking the allowlist, so every vault gym tool call was blocked and no environment-backed vault gym case could pass. List the concrete tool names from cli-first-tool-schemas.json instead; scoring paths keep the emitted command strings, which is what they match against. The vault gym tests still built the pre-CLI {"calls": [...]} payload and passed PromptCase.expected_tools, which no longer exists. They now emit one call in the configured default tool-call format (wrapper and required fields from tool_call_formats.yaml, IDs from the case's expected context) with the scenario's preferred CLI command paths, and assert the executed concrete tools. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013etFNjLuo22CCoQmUGYqCT
These tests predate be1220a/6dcb543f (CLI wrapper payloads, wrapper-less built-in default) and aabe1f4 (assertion-driven evaluation, no PromptCase.expected_tools); all of them have failed since those commits. - Assistant payloads use the configured default format from SynthChat/config/tool_call_formats.yaml: its wrapper name, its required argument fields and CLI command strings, instead of the removed context/calls shape and a hard-coded useTools. - Tool scenarios generate assistant turns through structured output, so the fake clients serve them as structured responses; the empty-reply retry is exercised on the structured path. - Multistep evaluator cases declare `correct` assertions on the final response, since a case without them can no longer pass, and assert the executed concrete tools. - Schema tests assert the current contract: the built-in default is native, the configured wrapper takes a JSON-string arguments payload that names the configured fields, and allowed tools are named in the generation prompt. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013etFNjLuo22CCoQmUGYqCT
c10120f made every protected HF stage require durable SUCCEEDED provisioning (claim event plus terminal event) in addition to a CONSUMABLE source transport. The shared _install_hf_source_transport fixture still attached evidence through record_provisioning_acknowledged, which never reaches SUCCEEDED, so the eval and loss stage runner tests stopped at "HF protected operation requires durable SUCCEEDED provisioning" before reaching the behaviour they check. The fixture now records the claim and its SUCCEEDED terminal event under the provisioning execution lock, the same chain the tracking-service tests use, binding the same evidence. No provider call is made. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013etFNjLuo22CCoQmUGYqCT
test_model_loader_source.py was added in 97e529e as a cherry-pick of the test half of 7a4e17c ("Add Qwen3.5 4B SFT launch path", origin/codex/qwen35-4b-launch). The implementation half (a FastVisionModel branch in Trainers/sft/src/model_loader.py and a Qwen3.5 4-bit warning in Trainers/sft/train_sft.py) never landed here, so both source-grep tests have failed ever since. Both files are now hash-locked members of the offline SFT worker closure, and this lineage supports Qwen3.5 another way. Replace the source greps with checks of that behaviour: - the locked loader loads Qwen3.5 through FastLanguageModel, keeps the processor a vision-language checkpoint returns, and the memory-efficient loss guard rejects a stock ForConditionalGeneration loss fallback; - every checked-in Qwen3.5 recipe and cloud experiment disables 4-bit loading, the configuration-level form of "no QLoRA for Qwen3.5". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013etFNjLuo22CCoQmUGYqCT
SynthChat/scripts/structured_document_bakeoff.py (7648833, after the census in d0c2b1a) calls structured_output on BaseLLMClient instances and was never classified, so the exact census failed. It follows the rules of the other classified consumers: clients come only from the shared shared.llm.create_client factory, which resolves the provider key by environment-variable name; the script reads no credential itself and makes no raw HTTP calls; and every completion is read only through .value and .usage. List it under BASE_CLIENT_CONSUMERS so the field-only consumption rule now covers it (it passes). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013etFNjLuo22CCoQmUGYqCT
TestReferenceCompleteness greps tests/ (among other roots) for Trainers/local/jobs and Trainers/cloud/jobs. The only hits are in tests/discovery/test_recipes.py itself, which has to spell the legacy paths to search for them, so both checks have failed since e671cdf. Drop hits from this module, both the repo-relative git-grep form and the absolute fallback form, and nothing else. A new check pins that the filter keeps a real hit elsewhere while dropping this module and docs/plans. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013etFNjLuo22CCoQmUGYqCT
The markdown ingestion acceptance test needs nested/beta.md to be a BOM + CRLF file, so it can check that ingestion normalises line endings. Under "* text=auto" the file was committed with LF line endings (64da4cc). It only had CRLF in a checkout that converts to CRLF, so the test failed on Linux and in CI. Mark that one path -text so Git stores and checks out its exact bytes on every platform, and renormalise it to BOM + CRLF. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013etFNjLuo22CCoQmUGYqCT
The qwen35_4b_token_profile.yaml example commits to the SHA-256 of modal-runtime-v1.lock.json, and the profiler refuses to run when the two differ. The example still carried the digest from fd28ab2; every reviewed lock refresh since 33dc682 has changed the lock, so the checked-in example could not run and test_checked_in_qwen_example_matches_current_profiler_contract failed. The lock is the authority: scripts/regenerate_modal_runtime_lock.py reports it CURRENT for the locked sources. No checked-in tool regenerates the example, so the new digest is computed from the lock bytes, the same digest the profiler checks, rather than typed in. The skill mirrors are re-synced with .skills/scripts/sync_skill_trees.py. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013etFNjLuo22CCoQmUGYqCT
This was referenced Oct 2, 2026
ProfSynapse
changed the base branch from
claude/sft-mask-doctor-port
to
feat/submodule-cloud-api-v1
October 2, 2026 23:28
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR 5 of 5. Stacked on #165.
Adds a CI workflow that runs the full test suite, and fixes the 84 failures it exposed. Before this, only a path-filtered conformance subset ran in CI.
CI, lint and pre-commit
.github/workflows/ci.yml:core,modal-inference-a-landmodal-inference-m-z.scripts/ci_pytest_shard.sh: runs a shard locally exactly the way CI does.F). All 1,462 existing findings are fixed. 25 hash-locked files keepper-file-ignores, so the inference and packaged-worker locks didn't need re-qualifying. Those entries are to be removed at the next deliberate lock refresh..pre-commit-config.yaml: ruff, check-yaml, check-toml, merge-conflict, large-files and actionlint. Nothing rewrites files.Real bugs fixed along the way
Trainers/sft/train_sft.py: an undefined_REPO_ROOTsilently disabled per-example loss computation for relative dataset paths. Fixed, with a regression test.shared/cloud_artifacts.py::sync_file_to_hf_bucket: usedrepo_type="bucket", which huggingface_hub rejects, so every single-file bucket push failed. It now usesbatch_bucket_files, like the provider adapter does.Evaluator/config/scenarios/vault_gym.yaml: the allowlist named CLI command strings, but the executor checks the concrete tool names. Every vault gym tool call was being blocked.SynthChat/workspace/sections.py: wrapper-less tool formats rendered "Use the `` wrapper for tool calls."SynthChat/scenarios/destructive.yaml: a tool rename had left each of fourtools:maps with a duplicatedstorageManager_archivekey. Each pair is collapsed to one entry.shared/flywheel/experiment_loop.py: the surrogate'sfeature_importancereturned integer split counts instead of floats.Evaluator/ui.py: the "... and N more failures" footer could never print.Test repairs, by cause
data_loadermodule-name collisions across trainers: newtests/trainers/_trainer_import.pyloads each trainer's modules under unique names.useToolsdefault removal;PromptCase.expected_toolsremoval;-text.SIGKILLs on Linux.skipif(os.name != "nt"), the repo's existing convention.Locks
train_sft.py,runtime_v1.pyandshared/cloud_artifacts.pychanged. Only the offline SFT worker closure and the Modal runtime lock were refreshed. The inference lock and the packaged-worker closure are untouched. All four checks report CURRENT.Test plan
Run with the CI shard script, on Python 3.12 with CPU torch:
core: 11,481 passed, 6 failed, 48 skipped.test_capture_hf_training_smoke_launcher_lock, which correctly refuses this sandbox's proxy and credential variables. All 7 tests in that file pass withenv -i.console_entrypoint,sft runtime_v1). They were shown to pass with a runner-style interpreter.modal-inference-a-l: 350 passed.modal-inference-m-z: 390 passed.ruff check .andpre-commit run --all-filesare clean.The workflow doesn't mark itself required. Make it a required check in branch protection once the first run is green.
🤖 Generated with Claude Code
https://claude.ai/code/session_013etFNjLuo22CCoQmUGYqCT
Generated by Claude Code