From 27303a3dee6d40cbc765442300785b3e5bacc659 Mon Sep 17 00:00:00 2001 From: Tarek Ziade Date: Tue, 25 Aug 2026 14:56:03 +0200 Subject: [PATCH] read the cutoff against the file that defines the base class (#56) `is_exempt_by_cutoff` keys on the file being linted. Whenever a rule resolves a base class into another model's directory, that is the wrong file: the verdict comes from the parent, but the violation lands on whichever model subclasses it, which is always the newer, non-exempt one. `DFineRepVggBlock(RTDetrRepVggBlock)` is a plain `nn.Module` because rt_detr says so, and d_fine's author cannot change that without editing a model their PR does not touch -- leaving only a model-wide allowlist entry or a suppression, both of which mute the rule for a model that did nothing wrong. `is_exempt_by_inherited_cutoff(defining_path, linted_path, cutoff_date)` takes the file that owns the structure. Inheriting inside your own model is never an excuse, so a same-model base grants nothing, and the parent's own file is still checked under the parent's own cutoff: the day the parent stops being grandfathered, both models are reported. TRF034 is the rule that walks cross-model bases today. Its base resolution now returns the file where the chain settles alongside the verdict, and skips a finding whose plain `nn.Module` belongs to a grandfathered model. `trf034`'s relative-import resolver moves to `_helpers.imported_classes`, since it is what makes a base class traceable to the model that owns it. Measured with every model treated as newly added while parents keep their real contribution dates -- the only mode in which the defect is visible, since neutralising the cutoff neutralises the inherited check too: 103 findings before, 87 after. The 16 are d_fine (3), deimv2 (3), tipsv2_dpt (3), pp_lcnet_v3, pp_ocrv5_server_rec, pp_ocrv6_small_rec, rf_detr, rt_detr_v2, sapiens2, slanet. `tipsv2_dpt` comes off the allowlist, where it was covering three inherited DPT layers. TRF057 is deliberately not changed: its modular findings are inherited in origin but local in fix -- `Glm4vForConditionalGeneration` and its `forward` are written in modular_glm4v.py, and the rule already says to add the decorator there -- so exempting them would drop the decorator from 42 brand-new public model classes. --- CHANGELOG.md | 15 +++++++++ docs/suppressing.md | 6 ++++ mlinter/_helpers.py | 39 ++++++++++++++++++++++++ mlinter/rules.toml | 4 +-- mlinter/trf034.py | 66 ++++++++++++++++++++++------------------ tests/rule_test_utils.py | 1 + tests/test_mlinter.py | 24 +++++++++++++++ tests/test_trf034.py | 62 ++++++++++++++++++++++++++++++++++++- 8 files changed, 184 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index adc524e..ef4edcf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,21 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), ### Changed +- A rule that resolves a base class into another model's directory now reads its `cutoff_date` against the file + where that base is **defined**, not the file being linted. `is_exempt_by_cutoff` keyed on the linted file, so + inherited structure was attributed to whichever model subclassed it -- always the newer, non-exempt one -- while + the model that actually owns the code stayed grandfathered, and the author's only ways out were a model-wide + allowlist entry or a suppression. `TRF034` is the rule that walks cross-model bases today and uses the new + `is_exempt_by_inherited_cutoff` helper: 16 findings go, all of the shape + `DFineRepVggBlock(RTDetrRepVggBlock)`, where the plain `nn.Module` is rt_detr's and d_fine cannot change it -- + `d_fine` (3), `deimv2` (3), `tipsv2_dpt` (3), `pp_lcnet_v3`, `pp_ocrv5_server_rec`, `pp_ocrv6_small_rec`, + `rf_detr`, `rt_detr_v2`, `sapiens2`, `slanet`. Measured with every model treated as newly added while parents + keep their real contribution dates, which is the only mode in which this defect is visible: 103 findings before, + 87 after. `tipsv2_dpt` came off the allowlist, where it was covering three inherited DPT layers. Nothing + enforced today is loosened: the parent's own file is still checked under the parent's own cutoff, so the day the + parent stops being grandfathered both models are reported. Closes + [#56](https://github.com/huggingface/transformers-mlinter/issues/56). + - Rewrote the `what_it_does` and `why_bad` prose in `rules.toml`, cutting it by a fifth overall and far more than that where it had run away: `TRF009` 3244 -> 1220 characters, `TRF041` 2418 -> 1457, `TRF038` 1940 -> 1301, `TRF042` 1620 -> 1032. No rule's explanation is over 1500 characters any more, down from 3244. What went is diff --git a/docs/suppressing.md b/docs/suppressing.md index 0cfac78..77935e6 100644 --- a/docs/suppressing.md +++ b/docs/suppressing.md @@ -90,6 +90,12 @@ read from the contribution date on the model's doc page. This is what keeps a ne ship with a 300-model allowlist. A model whose doc page has no contribution date **is** checked, so a missing date never silently disables a rule. +A rule that resolves a base class into another model's directory reads the cutoff against the file +where that base is **defined**, not the file being linted. Otherwise the violation lands on whichever +model subclasses the older one — always the newer, non-exempt one — and its author cannot fix it +without editing a model their PR does not touch. The parent's own file is still checked under the +parent's own cutoff, so nothing that is enforced today is loosened. + **Model allowlists.** Individual models that predate a convention and cannot be fixed without breaking backward compatibility are listed by name in `allowlist_models`. Each rule page on this site lists its own allowlist. diff --git a/mlinter/_helpers.py b/mlinter/_helpers.py index 1ca2f0b..c2b3500 100644 --- a/mlinter/_helpers.py +++ b/mlinter/_helpers.py @@ -140,6 +140,45 @@ def is_exempt_by_cutoff(file_path: Path, cutoff_date: str) -> bool: return contribution_date is not None and contribution_date < date.fromisoformat(cutoff_date) +def is_exempt_by_inherited_cutoff(defining_path: Path, linted_path: Path, cutoff_date: str) -> bool: + """Whether the structure being reported was authored in another model that the cutoff grandfathers. + + `is_exempt_by_cutoff` keys on the file being linted, which is the wrong file whenever a rule + resolves a base class into another model's directory: the verdict comes from the parent, but the + violation lands on whichever model subclasses it -- always the newer, non-exempt one. The author of + the new model cannot fix it without editing a model their PR does not touch, so the only ways out + are a model-wide allowlist entry or a suppression, both of which mute the rule for a model that did + nothing wrong. + + A rule that walks cross-model bases passes the file where the offending structure is defined. This + loosens nothing: the parent's own file is still checked under the parent's own cutoff, so the day + the parent stops being grandfathered both models are reported. + """ + if _model_dir_name(defining_path) == _model_dir_name(linted_path): + return False + return is_exempt_by_cutoff(defining_path, cutoff_date) + + +def imported_classes(tree: ast.Module, file_path: Path) -> dict[str, tuple[Path, str]]: + """Names bound by relative imports, mapped to the file they come from and their name there. + + Only relative imports are resolved, since that is how one model file reaches another + (`from ..llama.modeling_llama import LlamaDecoderLayer`), and they are what makes a base class + traceable to the model that owns it. + """ + imports: dict[str, tuple[Path, str]] = {} + for node in tree.body: + if not isinstance(node, ast.ImportFrom) or node.level == 0 or node.module is None: + continue + base_dir = file_path.parent + for _ in range(node.level - 1): + base_dir = base_dir.parent + imported_path = base_dir.joinpath(*node.module.split(".")).with_suffix(".py") + for alias in node.names: + imports[alias.asname or alias.name] = (imported_path, alias.name) + return imports + + def _has_rule_suppression(lines: list[str], rule_id: str, line_number: int) -> bool: if line_number <= 0: return False diff --git a/mlinter/rules.toml b/mlinter/rules.toml index e9871a0..dbe8b5d 100644 --- a/mlinter/rules.toml +++ b/mlinter/rules.toml @@ -600,11 +600,11 @@ diff = ''' [rules.TRF034] description = "Layer classes held in an nn.ModuleList must subclass GradientCheckpointingLayer." default_enabled = true -allowlist_models = ["cosmos3_edge", "dinov3_convnext", "hunyuan_vl", "kimi_k25", "openai", "radio", "tipsv2", "tipsv2_dpt", "x_clip", "xcodec2"] +allowlist_models = ["cosmos3_edge", "dinov3_convnext", "hunyuan_vl", "kimi_k25", "openai", "radio", "tipsv2", "x_clip", "xcodec2"] cutoff_date = "2026-06-20" [rules.TRF034.explanation] -what_it_does = "In modeling_*.py and modular_*.py, flags a locally-defined class whose name ends in `Layer` or `Block`, instantiated inside an `nn.ModuleList(...)`, that does not reach `GradientCheckpointingLayer` through its base chain. In modular files, relative imports into sibling model files are followed first; chains that still cannot be resolved are inconclusive. ModuleLists of projections, heads or experts are out of scope: they are not checkpointing boundaries." +what_it_does = "In modeling_*.py and modular_*.py, flags a locally-defined class whose name ends in `Layer` or `Block`, instantiated inside an `nn.ModuleList(...)`, that does not reach `GradientCheckpointingLayer` through its base chain. In modular files, relative imports into sibling model files are followed first; chains that still cannot be resolved are inconclusive. When the chain settles in another model's file -- the base class is that model's, and so is the missing `GradientCheckpointingLayer` -- the cutoff is read against that file, so a model is never asked to fix a base class it does not own. ModuleLists of projections, heads or experts are out of scope: they are not checkpointing boundaries." why_bad = "`gradient_checkpointing_enable()` wraps a layer only if it is a GradientCheckpointingLayer. A plain nn.Module in the stack is skipped silently, so training appears checkpointed while still allocating full activations for those layers, and the OOM shows up far from the cause." diff = ''' -class AcmeDecoderLayer(nn.Module): diff --git a/mlinter/trf034.py b/mlinter/trf034.py index d4c369c..0ba985e 100644 --- a/mlinter/trf034.py +++ b/mlinter/trf034.py @@ -22,7 +22,9 @@ _collect_class_bases, _has_rule_suppression, full_name, + imported_classes, is_exempt_by_cutoff, + is_exempt_by_inherited_cutoff, ) @@ -35,20 +37,6 @@ _MAX_INHERITANCE_HOPS = 12 -def _imported_classes(tree: ast.Module, file_path: Path) -> dict[str, tuple[Path, str]]: - imports: dict[str, tuple[Path, str]] = {} - for node in tree.body: - if not isinstance(node, ast.ImportFrom) or node.level == 0 or node.module is None: - continue - base_dir = file_path.parent - for _ in range(node.level - 1): - base_dir = base_dir.parent - imported_path = base_dir.joinpath(*node.module.split(".")).with_suffix(".py") - for alias in node.names: - imports[alias.asname or alias.name] = (imported_path, alias.name) - return imports - - def _parse_file(path: Path, cache: dict[Path, ast.Module | None]) -> ast.Module | None: if path not in cache: try: @@ -65,48 +53,60 @@ def _subclasses_gradient_checkpointing_layer( cache: dict[Path, ast.Module | None], seen: set[tuple[Path, str]] | None = None, hops: int = 0, -) -> bool | None: - """Return True if the chain reaches GradientCheckpointingLayer, False if fully resolved, None if not.""" +) -> tuple[bool | None, Path]: + """Whether the chain reaches GradientCheckpointingLayer, and which file settles the question. + + True if it reaches it, False if the chain resolves without one, None if some base could not be + followed. The second element is the file that owns the answer: for a False verdict, the file + defining the topmost ancestor that stops at `nn.Module` -- which is where the base class would have + to change, and so which model the violation belongs to. `DFineRepVggBlock(RTDetrRepVggBlock)` is a + plain module because rt_detr says so, not because d_fine did anything. + """ if seen is None: seen = set() key = (file_path, name) if key in seen or hops >= _MAX_INHERITANCE_HOPS: - return None + return None, file_path seen.add(key) class_to_bases = _collect_class_bases(tree) - imports = _imported_classes(tree, file_path) + imports = imported_classes(tree, file_path) if name not in class_to_bases: - return None + return None, file_path found_unknown = False + owner = file_path for base in class_to_bases[name]: simple = base.split(".")[-1] if simple == "GradientCheckpointingLayer": - return True + return True, file_path if base.startswith(("nn.", "torch.nn.")) or simple in {"Module", "object"}: continue if simple in class_to_bases: - resolved = _subclasses_gradient_checkpointing_layer(simple, tree, file_path, cache, seen, hops + 1) + resolved, resolved_owner = _subclasses_gradient_checkpointing_layer( + simple, tree, file_path, cache, seen, hops + 1 + ) elif simple in imports: imported_path, imported_name = imports[simple] imported_tree = _parse_file(imported_path, cache) - resolved = ( - None - if imported_tree is None - else _subclasses_gradient_checkpointing_layer( + if imported_tree is None: + resolved, resolved_owner = None, imported_path + else: + resolved, resolved_owner = _subclasses_gradient_checkpointing_layer( imported_name, imported_tree, imported_path, cache, seen, hops + 1 ) - ) else: - resolved = None + resolved, resolved_owner = None, file_path if resolved is True: - return True + return True, resolved_owner if resolved is None: found_unknown = True + elif owner == file_path: + # The first base that resolves to a plain module is the one to name. + owner = resolved_owner - return None if found_unknown else False + return (None, file_path) if found_unknown else (False, owner) def check(tree: ast.Module, file_path: Path, source_lines: list[str]) -> list[Violation]: @@ -137,9 +137,15 @@ def check(tree: ast.Module, file_path: Path, source_lines: list[str]) -> list[Vi continue if layer_name in reported: continue - inheritance_status = _subclasses_gradient_checkpointing_layer(layer_name, tree, file_path, parsed_files) + inheritance_status, owner_path = _subclasses_gradient_checkpointing_layer( + layer_name, tree, file_path, parsed_files + ) if inheritance_status is not False: continue + # The base class is another model's, and that model is grandfathered: reporting it here + # asks this author to edit a model their PR does not touch. + if is_exempt_by_inherited_cutoff(owner_path, file_path, CUTOFF_DATE): + continue if _has_rule_suppression(source_lines, RULE_ID, node.lineno): continue reported.add(layer_name) diff --git a/tests/rule_test_utils.py b/tests/rule_test_utils.py index a722079..989d64b 100644 --- a/tests/rule_test_utils.py +++ b/tests/rule_test_utils.py @@ -25,6 +25,7 @@ from mlinter import trf020 as _trf020_mod # noqa: F401 - re-exported for existing rule tests from mlinter import trf022 as _trf022_mod # noqa: F401 - re-exported for existing rule tests from mlinter import trf023 as _trf023_mod # noqa: F401 - re-exported for existing rule tests +from mlinter import trf034 as _trf034_mod # noqa: F401 - re-exported for existing rule tests from mlinter import trf038 as _trf038_mod # noqa: F401 - re-exported for existing rule tests from mlinter import trf042 as _trf042_mod # noqa: F401 - re-exported for existing rule tests from mlinter import trf057 as _trf057_mod # noqa: F401 - re-exported for existing rule tests diff --git a/tests/test_mlinter.py b/tests/test_mlinter.py index cc5164a..0b6b469 100644 --- a/tests/test_mlinter.py +++ b/tests/test_mlinter.py @@ -17,6 +17,7 @@ import tempfile import unittest from contextlib import redirect_stderr, redirect_stdout +from datetime import date from io import StringIO from pathlib import Path from types import SimpleNamespace @@ -813,6 +814,29 @@ def test_main_warns_about_an_explicit_file_no_rule_applies_to(self): self.assertEqual(exit_code, 0) self.assertIn("is not a model integration file", stderr.getvalue().replace("\n", "")) + def test_inherited_cutoff_exemption_reads_the_defining_model_not_the_linted_one(self): + models_root = Path("src/transformers/models") + parent = models_root / "rt_detr" / "modeling_rt_detr.py" + child = models_root / "d_fine" / "modular_d_fine.py" + + def contribution_date(path): + return date(2024, 1, 1) if "rt_detr" in str(path) else date(2026, 12, 1) + + with patch.object(_helpers_mod, "model_contribution_date", side_effect=contribution_date): + # The child is far too new to be grandfathered, but the structure is the parent's. + self.assertFalse(_helpers_mod.is_exempt_by_cutoff(child, "2026-06-20")) + self.assertTrue(_helpers_mod.is_exempt_by_inherited_cutoff(parent, child, "2026-06-20")) + # Inheriting inside your own model is never an excuse, whatever the model's date. + self.assertFalse( + _helpers_mod.is_exempt_by_inherited_cutoff( + models_root / "rt_detr" / "modular_rt_detr.py", parent, "2026-06-20" + ) + ) + # A parent the cutoff does not cover can be fixed, so it grants nothing. + self.assertFalse(_helpers_mod.is_exempt_by_inherited_cutoff(child, parent, "2026-06-20")) + # No cutoff configured means no exemption at all. + self.assertFalse(_helpers_mod.is_exempt_by_inherited_cutoff(parent, child, "")) + def test_known_model_dirs_is_empty_outside_a_transformers_checkout(self): with patch.object(_helpers_mod, "MODELS_ROOT", Path("/nonexistent/src/transformers/models")): self.assertEqual(_helpers_mod._known_model_dirs(), set()) diff --git a/tests/test_trf034.py b/tests/test_trf034.py index 19fe7a1..2c56beb 100644 --- a/tests/test_trf034.py +++ b/tests/test_trf034.py @@ -13,7 +13,7 @@ # limitations under the License. -from tests.rule_test_utils import Path, RuleTestCase, mlinter, tempfile +from tests.rule_test_utils import Path, RuleTestCase, _helpers_mod, _trf034_mod, date, mlinter, patch, tempfile class TRF034Test(RuleTestCase): @@ -50,6 +50,66 @@ def __init__(self, config): """ self.assertEqual(self._run(mlinter.TRF034, source), []) + def test_trf034_does_not_report_a_base_class_a_grandfathered_model_owns(self): + """The parent model owns the structure, so the cutoff has to be read against the parent's file.""" + with tempfile.TemporaryDirectory() as tmp_dir: + models_root = Path(tmp_dir) / "src" / "transformers" / "models" + (models_root / "rt_detr").mkdir(parents=True) + (models_root / "d_fine").mkdir() + (models_root / "rt_detr" / "modeling_rt_detr.py").write_text( + "class RTDetrRepVggBlock(nn.Module):\n pass\n", encoding="utf-8" + ) + modular_path = models_root / "d_fine" / "modular_d_fine.py" + source = """ +from ..rt_detr.modeling_rt_detr import RTDetrRepVggBlock + + +class DFineRepVggBlock(RTDetrRepVggBlock): + pass + + +class DFineEncoder(nn.Module): + def __init__(self, config): + super().__init__() + self.blocks = nn.ModuleList([DFineRepVggBlock(config) for _ in range(2)]) +""" + + def run(parent_date): + # The model being linted has no contribution date, so it is never grandfathered itself. + def contribution_date(path): + return parent_date if "rt_detr" in str(path) else None + + with ( + patch.object(_helpers_mod, "MODELS_ROOT", models_root), + patch.object(_helpers_mod, "model_contribution_date", side_effect=contribution_date), + patch.object(_trf034_mod, "CUTOFF_DATE", "2026-06-20"), + ): + violations = mlinter.analyze_file(modular_path, source, enabled_rules={mlinter.TRF034}) + return [violation for violation in violations if violation.rule_id == mlinter.TRF034] + + # rt_detr predates the cutoff: d_fine cannot fix RTDetrRepVggBlock, so nothing is reported. + self.assertEqual(run(date(2024, 1, 1)), []) + # A parent the cutoff does not cover is a parent that can be fixed, so the finding stands. + self.assertEqual(len(run(date(2026, 7, 1))), 1) + + def test_trf034_still_reports_a_base_owned_by_the_model_being_linted(self): + """Inheriting inside your own model is no excuse, whatever the model's own date says.""" + source = """ +class FooBaseLayer(nn.Module): + pass + + +class FooDecoderLayer(FooBaseLayer): + pass + + +class FooModel(FooPreTrainedModel): + def __init__(self, config): + super().__init__(config) + self.layers = nn.ModuleList([FooDecoderLayer(config) for _ in range(2)]) +""" + self.assertEqual(len(self._run(mlinter.TRF034, source)), 1) + def test_trf034_follows_local_inheritance(self): source = """ class FooBaseLayer(GradientCheckpointingLayer):