Skip to content

[RFC] add class support in bc-linter #6953

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 13, 2025

Conversation

zhewenl
Copy link
Contributor

@zhewenl zhewenl commented Jul 25, 2025

as discussed with @izaitsevfb, we want to also include linter checks on classes in addition to public functions; but prior to landing this PR, we need to create some rules template for linters, where we can define what to check(or not).
Like:

pytorch:
- Files: global
- Include: functions

vLLM:
- Files: vllm/v1/attrition, vllm/v1/core
- Include, classes, functions

cc @huydhn @yangw-dev @houseroad

Copy link

vercel bot commented Jul 25, 2025

@zhewenl is attempting to deploy a commit to the Meta Open Source Team on Vercel.

A member of the Team first needs to authorize it.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 25, 2025
@zhewenl zhewenl changed the title add class [do not commit] add class support in bc-linter Jul 25, 2025
@zhewenl zhewenl changed the title [do not commit] add class support in bc-linter [RFC] add class support in bc-linter Jul 25, 2025
@izaitsevfb
Copy link
Contributor

Ran the new version retroactively on the recent PyTorch history (4000 commits).

Raw findings:

- e65ab9a868aff9397a695085b3f8a2199724771c vs bfe6765d6b621f60e3dfd6d891d0177bc1
5d5e6c
  - File: torchgen/gen_aoti_c_shim.py
  - Violation: FieldTypeChanged in class ShimGenerator, field dispatch_key
  - Diff: Shows typing import and generator changes; type of a dataclass field a
ppears to have changed.
  - PR: 158974
  - Labels: includes suppress-bc-linter

- 02ca965560e74e44dfd446d97b77d84eb35b2de2 vs 511d9873789b11e7c84fcfdfffa9166faa
0d83b3
  - File: torch/distributed/checkpoint/staging.py
  - Violation: FieldRemoved in class StagingOptions, field use_cuda_non_blocking
_copy (renamed to use_non_blocking_copy)
  - Diff: Renames option name; our linter sees it as removal.
  - PR: 158337
  - Labels: no suppress-bc-linter label

- f76f4abf3f10bd36a47e7cebdce90290ce76e564 vs be483a54817fbfbf184af363bf9469d01b
fa15ef
  - File: tools/stats/monitor.py
  - Violations: FieldAdded x3 in class GpuData (allocated_mem, allocated_mem_val
ue, total_mem_value)
  - Diff: Adds three dataclass fields with no defaults; our policy flags require
d dataclass field additions.
  - PR: 156907
  - Labels: no suppress-bc-linter label
  - Note: This is outside core library (tools/), but still detected.

- 250ae2531c55dcc50f558ec739941324e3f9a4d4 vs 011026205a9d4c38458130f8ca242028f6
184bf0
  - File: torch/cuda/graphs.py
  - Violation: FieldTypeChanged in class graph, field default_capture_stream
  - Diff: Large typing cleanup; type for the field likely changed.
  - PR: 158192
  - Labels: no suppress-bc-linter label (topic: not user facing)

- 4283d96bcdf9aaa4289985267186d74ba6534ee5 vs b4476ca378be50034bd5cdc1eaa9510433
7c998a
  - File: setup.py
  - Violation: ClassDeleted for wheel_concatenate (build-time class removed)
  - Diff: Removes external wheel.bdist_wheel shim usage.
  - PR: 157783
  - Labels: includes suppress-bc-linter

Some observations:

  • not many class violations in PyTorch codebase.
  • reported violations are on-point (though some are on internal surface)
  • no noticeable regression in bc-linter behavior

@izaitsevfb izaitsevfb merged commit 5382f4d into pytorch:main Aug 13, 2025
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants