Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 6 additions & 0 deletions docs/suppressing.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
39 changes: 39 additions & 0 deletions mlinter/_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions mlinter/rules.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
66 changes: 36 additions & 30 deletions mlinter/trf034.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@
_collect_class_bases,
_has_rule_suppression,
full_name,
imported_classes,
is_exempt_by_cutoff,
is_exempt_by_inherited_cutoff,
)


Expand All @@ -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:
Expand All @@ -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]:
Expand Down Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions tests/rule_test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
24 changes: 24 additions & 0 deletions tests/test_mlinter.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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())
62 changes: 61 additions & 1 deletion tests/test_trf034.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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):
Expand Down