Skip to content

read the cutoff against the file that defines the base class (#56) - #63

Open
tarekziade wants to merge 1 commit into
mainfrom
fix/trf056-inherited-cutoff
Open

tarekziade wants to merge 1 commit into
mainfrom
fix/trf056-inherited-cutoff

Conversation

@tarekziade

Copy link
Copy Markdown
Collaborator

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.

`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.

@vasqu vasqu left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is the right thing to do tbh, my main concern would be that we would be hiding even more rather than fix it. If this would be integrated from the get go I would never have discovered those false positives if that makes sense

It might make sense if the cutoff date is not too recent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants