tests: public-api surface drift detector (companion to test_import_fixes_drift.py)#5428
Conversation
Companion to tests/test_import_fixes_drift.py (PR #5414): that file catches drift in THIRD-PARTY libs (transformers / trl / triton / peft / vllm / torchcodec / xformers); this file catches drift in unsloth's OWN public-surface API -- the top-9 classmethods + symbols that unslothai/notebooks calls at ~2000 cumulative sites. Closes the gap where a refactor on this repo (e.g. renaming FastLanguageModel.from_pretrained -> .load) would pass unsloth CI green and surface only on the next unslothai/notebooks CI run, or worse, on a user's Colab crash report. Coverage (call-site counts measured against unslothai/notebooks main): test_fast_language_model_class_present test_fast_language_model_from_pretrained_kwargs 506 sites test_fast_language_model_get_peft_model_kwargs 304 sites test_fast_language_model_for_inference_callable 370 sites test_fast_vision_model_class_and_methods (4 methods) test_fast_vision_model_get_peft_model_vision_kwargs (4 kwargs) test_fast_model_class_and_methods (2 methods) test_fast_model_from_pretrained_kwargs 103 sites test_is_bf16_supported_or_alias_callable 48 + 8 sites Each test asserts the healthy public shape via inspect.signature; on regression fires pytest.fail("DRIFT DETECTED: ...") -- never pytest.skip -- so the Core matrix cell goes red. Mirrors the same skeleton used by tests/test_import_fixes_drift.py. Wired as a new step in consolidated-tests-ci.yml right after the import_fixes drift step, inside every Core matrix cell. Local verification on transformers 4.57.6 + unsloth main: pytest tests/test_public_api_surface.py -v -> 9 passed in 0.02s
for more information, see https://pre-commit.ci
![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: 9cf0d401cd
ℹ️ 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".
| unsloth = pytest.importorskip("unsloth") | ||
| has_new = callable(getattr(unsloth, "is_bf16_supported", None)) | ||
| has_old = callable(getattr(unsloth, "is_bfloat16_supported", None)) | ||
| if not (has_new or has_old): |
There was a problem hiding this comment.
When only one alias is exported, this test still passes even though the other import form remains part of the public surface: the docstring cites notebook call sites for both is_bf16_supported and is_bfloat16_supported, and repo-wide search shows tests importing from unsloth import is_bf16_supported. In the scenario where only the legacy name remains (or only the new name remains), those imports crash while this new drift detector stays green, so this should assert both public names are callable unless the removed name is deliberately no longer supported.
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 adds a new test suite in tests/test_public_api_surface.py to detect breaking changes in the public API of the unsloth library by checking for the existence of critical classes and their expected method signatures. Review feedback suggests removing the unused _signature_param_names helper function and hardening the _accepts function to correctly report drift if an object's signature cannot be inspected.
|
|
||
| def _signature_param_names(callable_obj) -> set[str]: | ||
| try: | ||
| sig = inspect.signature(callable_obj) | ||
| except (TypeError, ValueError): | ||
| return set() |
There was a problem hiding this comment.
The helper function _signature_param_names is defined but not used anywhere in this test file. It should be removed to keep the codebase clean and avoid maintaining dead code.
| sig = inspect.signature(callable_obj) | ||
| except (TypeError, ValueError): |
There was a problem hiding this comment.
Returning True, set() when inspect.signature fails is too permissive for a drift detector. If the object's signature cannot be inspected (for example, if the attribute was replaced by a non-callable value like None), the test would silently pass. Returning False and the set of required arguments ensures that such regressions are correctly identified as drift. Consider centralizing this signature validation logic into a helper function to ensure consistency across the codebase.
| sig = inspect.signature(callable_obj) | |
| except (TypeError, ValueError): | |
| except (TypeError, ValueError): | |
| return False, kwargs |
References
- Centralize recurring or complex logical checks into a single helper function and reuse it across the codebase to ensure consistency and simplify maintenance.
1 participant
Summary
Adds
tests/test_public_api_surface.py, a slim companion to PR #5414'stests/test_import_fixes_drift.py. That file catches drift in THIRD-PARTY libraries (transformers / trl / triton / peft / vllm / torchcodec / xformers); this one catches drift in unsloth's OWN public-surface API -- the top-9 classmethods + symbols thatunslothai/notebookscalls at ~2000 cumulative sites.Closes the gap where a refactor on this repo (e.g. renaming
FastLanguageModel.from_pretrained -> .load) would pass unsloth CI green and surface only on the nextunslothai/notebooksCI run, or worse, on a user's Colab crash report.Coverage
Call-site counts measured against
unslothai/notebooksmain:test_fast_language_model_class_presentFastLanguageModeltest_fast_language_model_from_pretrained_kwargs.from_pretrainedaccepts{model_name, max_seq_length, dtype, load_in_4bit}test_fast_language_model_get_peft_model_kwargs.get_peft_modelaccepts{r, lora_alpha, lora_dropout, target_modules, bias, use_gradient_checkpointing, random_state}test_fast_language_model_for_inference_callable.for_inferencetest_fast_vision_model_class_and_methodsFastVisionModel+ 4 methodstest_fast_vision_model_get_peft_model_vision_kwargs.get_peft_modelaccepts vision-LoRA kwargs{finetune_vision_layers, finetune_language_layers, finetune_attention_modules, finetune_mlp_modules}test_fast_model_class_and_methodsFastModel+ 2 methodstest_fast_model_from_pretrained_kwargstest_is_bf16_supported_or_alias_callableEach test asserts the healthy public shape via
inspect.signature; on regression firespytest.fail("DRIFT DETECTED: ...")(neverpytest.skip) so the Core matrix cell goes red. Mirrors the same skeleton used bytests/test_import_fixes_drift.py.CI wiring
Wired as a new step in
.github/workflows/consolidated-tests-ci.ymlright after theimport_fixes drift detectorsstep. Runs inside every Core matrix cell.Local verification
Test plan
pytest tests/test_public_api_surface.pyis 9/9 locally on transformers 4.57.6 + unsloth main.