tests: pinned-symbol canary for unsloth-zoo save_pretrained_merged guards (#5410)#5433
Conversation
![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: 6860ec9143
ℹ️ 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 src is None: | ||
| pytest.skip("unsloth_zoo/saving_utils.py not fetchable") |
There was a problem hiding this comment.
If unsloth-zoo later renames or removes unsloth_zoo/saving_utils.py, fetch_text returns None for the 404 and this helper skips every saving-utils guard test, so the version-compat job stays green exactly when the canary has stopped checking the module that unsloth/save.py imports for merge_and_overwrite_lora. For this pinned-symbol test, a missing upstream file should be a failure (or checked via an explicit alternate path), not a skip.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new compatibility test file, test_unsloth_zoo_save_merged_pinned_symbols.py, which serves as a canary to prevent regressions of MoE lora merging fixes in the unsloth-zoo repository. The tests perform static analysis on fetched source code to verify the presence of specific guard symbols, layout detectors, and entry points. Feedback suggests improving the robustness of the static checks by using more flexible regex for bounded loop detection and more specific regex patterns for identifying function definitions and calls to avoid false positives or negatives.
| "Shard-local num_experts will produce silent fallbacks again." | ||
| ) | ||
| # The walk must be bounded (PR #647 hardening). A bare `while module | ||
| # is not None` without a visited / depth bound regresses to an |
There was a problem hiding this comment.
The regex for detecting a bounded loop is overly restrictive. It specifically requires the loop variable to be _ and the range limit to be a literal integer. This will cause the test to fail if the upstream code uses a different variable name (e.g., for i in range(...)) or a constant/variable for the limit (e.g., range(MAX_DEPTH)). A more robust check would simply look for the presence of a for ... in range( construct to ensure the loop is bounded.
| # is not None` without a visited / depth bound regresses to an | |
| bounded_re = re.compile(r"for\s+\w+\s+in\s+range\s*\(") |
| if not save_py.is_file(): | ||
| pytest.skip(f"{save_py} not present (sparse checkout?)") | ||
| text = save_py.read_text(encoding = "utf-8", errors = "replace") | ||
| assert ( | ||
| "save_pretrained_merged" in text | ||
| ), "save_pretrained_merged entry point removed from unsloth/save.py." | ||
| # One of the two dispatch helpers must still call into unsloth_zoo's | ||
| # merge_and_overwrite_lora; renaming both would silently bypass #647. | ||
| assert "merge_and_overwrite_lora" in text, ( | ||
| "Neither unsloth_save_pretrained_merged nor " |
There was a problem hiding this comment.
The assertions for checking the entry point and dispatch call are too permissive. Using "string" in text can result in false positives if the strings appear in comments, docstrings, or imports without the actual logic being present. It is better to use regex to ensure these are actual function definitions or calls.
| if not save_py.is_file(): | |
| pytest.skip(f"{save_py} not present (sparse checkout?)") | |
| text = save_py.read_text(encoding = "utf-8", errors = "replace") | |
| assert ( | |
| "save_pretrained_merged" in text | |
| ), "save_pretrained_merged entry point removed from unsloth/save.py." | |
| # One of the two dispatch helpers must still call into unsloth_zoo's | |
| # merge_and_overwrite_lora; renaming both would silently bypass #647. | |
| assert "merge_and_overwrite_lora" in text, ( | |
| "Neither unsloth_save_pretrained_merged nor " | |
| assert re.search(r"def\s+\w*save_pretrained_merged\b", text), ( | |
| "save_pretrained_merged entry point removed from unsloth/save.py." | |
| ) | |
| # One of the two dispatch helpers must still call into unsloth_zoo's | |
| # merge_and_overwrite_lora; renaming both would silently bypass #647. | |
| assert re.search(r"merge_and_overwrite_lora\s*\(", text), ( | |
| "Neither unsloth_save_pretrained_merged nor " | |
| "unsloth_generic_save_pretrained_merged calls " | |
| "merge_and_overwrite_lora; the unsloth-zoo fix is bypassed." | |
| ) |
danielhanchen
force-pushed
the
ci-save-merged-smoke-5410
branch
from
6f35f75 to
0d08260
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 995a31ab9f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| assert re.search( | ||
| r"for\s+_\s+in\s+range\s*\(\s*\d+\s*\)", src | ||
| ), "resolver walk is no longer bounded by `for _ in range(N):`." |
There was a problem hiding this comment.
This assertion only checks for any for _ in range(<digits>) anywhere in saving_utils.py, so it can pass even if _resolve_num_experts_from_lora_stats regresses to an unbounded walk (for example, while traversal) as long as an unrelated bounded loop exists elsewhere in the file. Because this test is meant to guard against hangs from cyclic base_layer chains, the check should be tied to the resolver function body (or a more specific pattern) to avoid false negatives.
Useful? React with 👍 / 👎.
| assert re.search( | ||
| r"raise\s+RuntimeError\b[^\n]*MoE", src, re.IGNORECASE | ||
| ), "no `raise RuntimeError(...MoE...)`; post-loop guard weakened." |
There was a problem hiding this comment.
The regex currently succeeds if any raise RuntimeError(...MoE...) appears anywhere in saving_utils.py, even if the post-merge fallback guard in the relevant code path is removed or weakened. That means this canary can report green while the specific safety check it claims to protect has regressed. Narrowing the match to the intended function/section would prevent this false-pass behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a313ede3a9
ℹ️ 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".
| "_resolve_num_experts_from_lora_stats", | ||
| ) | ||
| ): | ||
| pytest.skip( |
There was a problem hiding this comment.
If unsloth-zoo has already landed #647 and a later refactor removes or renames all three sentinel symbols at once, this condition routes every saving-utils guard test back through the “#647 has not yet merged” skip path, so the canary goes green on the exact regression it is meant to catch. The temporary skip needs a separate, time-bounded/explicit merge-state check, or a missing post-merge guard set should fail instead of skipping.
Useful? React with 👍 / 👎.
| if "test_merge_moe_gate_expert_standard_layout" not in src: | ||
| pytest.skip("unslothai/unsloth-zoo#647 not yet merged; coverage appears later.") |
There was a problem hiding this comment.
After #647 has landed, deleting or renaming test_merge_moe_gate_expert_standard_layout is one of the regressions this canary claims to detect, but this branch treats that exact absence as “not yet merged” and skips the entire upstream test-coverage check. That lets the standard-layout coverage disappear while CI remains green; the pre-merge skip should not be keyed to a test name that later must be enforced.
Useful? React with 👍 / 👎.
…ards (#5410) unsloth#5410 was a class of silent-write bug in the save_pretrained_merged path that the existing CI matrix could not detect because the merge-helper tests were not wired through the upstream-drift suite. The full fix lives in unslothai/unsloth-zoo#647 (layout-aware MoE merge helpers, authoritative num_experts resolver, loud-fail counter, generation_config.json save). This PR adds the unsloth-side canary that watches for the four guards staying in place in unsloth-zoo so a future refactor cannot silently regress them. tests/version_compat/test_unsloth_zoo_save_merged_pinned_symbols.py fetches unsloth_zoo/saving_utils.py + tests/test_unsloth_zoo_lora_merge.py from unslothai/unsloth-zoo:main and asserts: - _MOE_MERGE_STATE / _reset_moe_merge_state / _record_moe_merge_fallback are still defined and a `raise RuntimeError(...MoE...)` still fires when fallback > 0. - _detect_moe_lora_layout exists and both "swapped" / "standard" branch labels are reachable in the source. - _resolve_num_experts_from_lora_stats is present AND its base_layer walk is bounded by `for _ in range(N):` (a cyclic ParamWrapper chain must not hang the merge). - merge_and_overwrite_lora still calls model.generation_config.save_pretrained(...). - tests/test_unsloth_zoo_lora_merge.py keeps the six PEFT 0.19+ standard-layout regression tests added in #647. - Local unsloth/save.py still names save_pretrained_merged and routes through merge_and_overwrite_lora (i.e. the entry point still reaches the upstream fix). While #647 is still open, the four symbol tests SKIP cleanly with a message naming #647. When #647 merges into unsloth-zoo main, the same tests automatically become hard gates and catch any future regression. The sixth test (local entry-point grep) passes today. CPU-only static fetch, ~0.1s. Wired into the existing peft-pinned-symbols job in .github/workflows/version-compat-ci.yml so it runs on every PR that touches unsloth/** and on the daily schedule. Local run: 1 passed, 5 skipped (expected; #647 open).
danielhanchen
force-pushed
the
ci-save-merged-smoke-5410
branch
from
a313ede to
fa20985
Compare
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9564c6cd70
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if src is None: | ||
| pytest.skip("tests/test_unsloth_zoo_lora_merge.py not fetchable") |
There was a problem hiding this comment.
When tests/test_unsloth_zoo_lora_merge.py is missing (rename/remove/404), this test currently calls pytest.skip, which leaves CI green even though the canary has lost visibility into the exact regression tests it is supposed to enforce. In practice, any upstream move of that file after #647 would silently disable this guard rather than reporting a breakage, so this branch should fail (or explicitly resolve alternate paths) once the sentinel coverage is expected to exist.
Useful? React with 👍 / 👎.
| assert re.search( | ||
| r"generation_config\.save_pretrained\s*\(", src |
There was a problem hiding this comment.
This regex passes if generation_config.save_pretrained( appears anywhere in saving_utils.py, including unrelated helpers or dead/commented code, so the canary can stay green even if the call is removed from the merge_and_overwrite_lora path that actually fixes the EOS-regression risk. Anchoring the check to the relevant function/section is needed to avoid false passes on refactors.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21318a88ee
ℹ️ 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".
| "_detect_moe_lora_layout" in src | ||
| ), "_detect_moe_lora_layout removed (issue #5410)." | ||
| assert ( | ||
| '"swapped"' in src and '"standard"' in src |
There was a problem hiding this comment.
If unsloth-zoo keeps the same semantic layout labels but formats them as Python single-quoted strings, e.g. 'swapped'/'standard', this hard gate fails even though the detector still supports both conventions. Because this test is meant to guard the labels rather than a source-formatting choice, it should match either quote style or parse the string literals.
Useful? React with 👍 / 👎.
| assert ( | ||
| "merge_and_overwrite_lora" in text | ||
| ), "no dispatch into unsloth_zoo merge; #647 bypassed." |
There was a problem hiding this comment.
This assertion passes whenever merge_and_overwrite_lora appears anywhere in unsloth/save.py, such as an unused import, a comment, or a different helper, so a future change can remove the save_pretrained_merged dispatch while this canary still stays green. Since the stated regression is the entry point bypassing the upstream fix, the check should verify a call inside the relevant save_pretrained_merged implementation rather than a file-wide substring.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e356bac61a
ℹ️ 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".
| assert re.search( | ||
| r"raise\s+RuntimeError\b[^\n]*MoE", src, re.IGNORECASE | ||
| ), "no `raise RuntimeError(...MoE...)`; post-loop guard weakened." |
There was a problem hiding this comment.
If the upstream MoE guard raises RuntimeError with the message on the next line — a common Black-wrapped form for long error strings, e.g. raise RuntimeError(\n "MoE ..."\n) — this regex does not match because [^ ]*MoE only scans the raise RuntimeError line. Once #647 lands, that equivalent guard would make this canary fail CI even though the protection is still present; the check should either allow newlines inside the call or inspect the call body more structurally.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b337038eff
ℹ️ 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".
| "test_moe_merge_fallback_counter_records_bad_layout", | ||
| "test_resolve_num_experts_walks_base_layer_chain", | ||
| ): | ||
| assert name in src, f"regression test `{name}` removed." |
There was a problem hiding this comment.
Once the upstream #647 tests exist, this canary still passes if one of these regression tests is deleted or disabled but its name remains in a comment, skip reason, parameter string, or helper list in tests/test_unsloth_zoo_lora_merge.py. Since this check is meant to enforce executable upstream coverage, it should verify a real def/async def for each expected test (for example via the existing has_def helper) rather than a file-wide substring.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 953f38b9e6
ℹ️ 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".
| assert re.search( | ||
| r"generation_config\.save_pretrained\s*\(", src |
There was a problem hiding this comment.
Now that unsloth-zoo#647 has merged, this hard gate will fail even though the fix is present: the merged upstream code assigns the generation config to gen_cfg and calls gen_cfg.save_pretrained(save_directory), so this literal generation_config.save_pretrained( regex never matches. In the post-#647 CI context this makes the newly added version-compat job red for a formatting/aliasing difference rather than a real regression.
Useful? React with 👍 / 👎.
zoo#647 landed two layout changes that broke the pinned-symbol
canary's exact-string regex matches but kept the underlying
guarantees intact:
- The post-loop MoE LoRA fallback `raise RuntimeError(...)` wraps
the "MoE" wording onto a second line; the old `[^\n]*` did not
cross newlines. Switch to `.*?` + re.DOTALL.
- The generation_config save now binds the attr to a local var
`gen_cfg = getattr(model, "generation_config", ...)` and calls
`gen_cfg.save_pretrained(save_directory)`, so a literal
`generation_config.save_pretrained(` substring no longer matches.
Anchor on the conceptual operation: a `generation_config` mention
followed (within a small char window) by a `.save_pretrained(`
call. That is what the canary actually cares about.
Verified locally:
pytest tests/version_compat/test_unsloth_zoo_save_merged_pinned_symbols.py
-> 2 passed (4 deselected)
1 participant
Summary
#5410 was a silent-write bug in the
save_pretrained_mergedpath that the existing CI matrix could not detect because the merge-helper tests were not wired through the upstream-drift suite. The full fix lives in unslothai/unsloth-zoo#647 (layout-aware MoE merge helpers, authoritativenum_expertsresolver, loud-fail counter,generation_config.jsonsave). This PR adds the unsloth-side canary that watches for the four guard families staying in place in unsloth-zoo so a future refactor cannot silently regress them.The companion CI work in unslothai/unsloth-zoo#649 wires the unsloth-zoo CI matrix to exercise the same guards.
What this PR adds
A single new test file at
tests/version_compat/test_unsloth_zoo_save_merged_pinned_symbols.pyand one workflow line in.github/workflows/version-compat-ci.yml.The test fetches
unsloth_zoo/saving_utils.pyandtests/test_unsloth_zoo_lora_merge.pyfromunslothai/unsloth-zoo:main(no pip install, no GPU, no model download) and asserts:_MOE_MERGE_STATE+_reset_moe_merge_state+_record_moe_merge_fallback+ theraise RuntimeError(...MoE...)_detect_moe_lora_layout+ both\"swapped\"and\"standard\"branch labels_resolve_num_experts_from_lora_stats+ the boundedfor _ in range(N):walknum_expertsnon-divisors silently fall back; cyclicmodule.base_layerhangsgeneration_config.save_pretrained(...)call inmerge_and_overwrite_loratests/test_unsloth_zoo_lora_merge.pyunsloth/save.pystill namessave_pretrained_mergedand referencesmerge_and_overwrite_loraBehaviour while unslothai/unsloth-zoo#647 is open
The four symbol tests detect that the guard markers are not yet in
unsloth-zoo:mainand SKIP cleanly with a message naming #647. The local entry-point test passes today.The skip condition is purely: "none of
_MOE_MERGE_STATE,_detect_moe_lora_layout, or_resolve_num_experts_from_lora_statsare in the fetched source". The moment #647 merges into unsloth-zoo main, the skip stops triggering and the same tests become hard gates. No follow-up PR or manual flip needed.Cost
Pure CPU static fetch, ~0.1s, wired into the existing
peft-pinned-symbolsjob. Runs on every PR that touchesunsloth/**plus the daily 06:43 UTC schedule.Related
save_pretrained_merged#5410 to shipTest plan
PYTHONPATH=. pytest tests/version_compat/test_unsloth_zoo_save_merged_pinned_symbols.py -v-> 1 passed, 5 skipped (expected while Load q4 models for finetuning #647 is open)unsloth/save.py