[BE] Enable C419 rule for any all shortcircuiting#99890
[BE] Enable C419 rule for any all shortcircuiting#99890Skylion007 wants to merge 3 commits intopytorch:mainfrom
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/99890
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ✅ No FailuresAs of commit ad98f63: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
github-actions
bot
added
ciflow/inductor
module: dynamo
module: inductor
release notes: quantization
Skylion007
force-pushed
the
skylion007/ruff-enable-C419
branch
from
95bae8f to
ad98f63
Compare
Skylion007
requested review from
a team,
BowenBao,
Chillee,
H-Huang,
abock,
awgu,
ezyang,
fegin,
kshitij12345,
kwen2501,
mrshenli,
rohan-varma,
wanchaol,
zhaojuanmao and
zou3519
as code owners
Skylion007
requested review from
albanD,
d4l3k,
fmassa,
jbschlosser,
jerryzh168,
kiukchung,
mikaylagawarecki,
soulitzer and
soumith
as code owners
|
@pytorchbot merge |
Merge failedReason: Approval needed from one of the following: |
malfet
left a comment
malfet
left a comment
There was a problem hiding this comment.
We should keep at least one test in JIT that checks, that all (if at all supported by JitScript) can accept list
Why? We have no special handling of any / all vs other builtins that already test list comprehensions. I could not find any unit tests for "any" / "all" specifically before this PR and none were changed. It should be fine as I don't think it should trigger any special behavior and no specific any/all tests were edited to my knowledge. @malfet was there a specific change to a unit test you wanted me to revert? |
If there are no specific handling, than it's probably fine as is. But haven't you mentioned that certain PR added list comprehension support to JITScript. If this is the case, it would be good to have at least one test that constructs a list inside the jited function and passes it to one of builtins. But I've scanned your change yesterday and couldn't spot a single test that you've modified that tested for this functionality. |
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
6 participants
Apparently #78142 made torch.JIT allow for simple generator expressions which allows us to enable rules that replace unnecessary list comprehensions with generators in any/all. This was originally part of #99280 but I split it off into this PR so that it can be easily reverted should anything break.
cc @soumith @voznesenskym @penguinwu @anijain2305 @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @Xia-Weiwen @wenzhe-nrv @jiayisunx @peterbell10 @desertfire