Skip to content
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

Request: allowlist setting for N812/N817 #12031

Closed
jamesbraza opened this issue Jun 25, 2024 · 6 comments · Fixed by #12033
Closed

Request: allowlist setting for N812/N817 #12031

jamesbraza opened this issue Jun 25, 2024 · 6 comments · Fixed by #12033
Assignees
Labels
bug Something isn't working

Comments

@jamesbraza
Copy link
Contributor

There are certain libraries like PyTorch's FSDP that standardized on being imported with all caps:

from torch.distributed.fsdp import FullyShardedDataParallel as FSDP

...
sharded_module = FSDP(my_module)

Currently, with Ruff 0.4.10 you just have to use a # noqa: N817 for every import line of libraries like FSDP.

Also, a similar PyTorch use case is from torch.nn import functional as F, which raises N812.

Ideally, there is a configuration option to build up an allowlist of possible names to not raise N812/N817. I tried using the ignore-names or extend-ignore-names settings, but neither worked.

Do you think we can either allow ignore-names to apply to N817, or create a new option ignore-acronyms that can accommodate this use case?

Thank you for your consideration!

@charliermarsh charliermarsh self-assigned this Jun 25, 2024
@charliermarsh
Copy link
Member

It does look like those rules respect the setting, but confusingly... N812 matches against the asname (FSDP above) while N817 matches against the name (FullyShardedDataParallel above). So I'll fix that. Which would you find more intuitive? Maybe we match against... both?

@jamesbraza
Copy link
Contributor Author

jamesbraza commented Jun 25, 2024

So to write it out, I think:

from torch.nn import functional as F:

  • Ruff should throw N812 as functional is lowercase
    • And one part of this request is to manually allowlist this in pyproject.toml
  • Ruff should not throw N817 as functional is not camel case

from torch.distributed.fsdp import FullyShardedDataParallel as FSDP

  • Ruff should not throw N812 as FullyShardedDataParallel is not lowercase
  • Ruff should throw N817 as this is N817's normal use case
    • And the other part of this request is to manually allowlist this in pyproject.toml

Does that answer your question?

@charliermarsh
Copy link
Member

Actually what I was asking is: given from torch.distributed.fsdp import FullyShardedDataParallel as FSDP, what value would you expect to put in ignore-names?

@jamesbraza
Copy link
Contributor Author

Oh I see now. Yeah I think I would want to put FSDP, as that's the infringing acronym

Open to specifying FullyShardedDataParallel, but that's not really the point of infringement so I prefer FSDP

@charliermarsh
Copy link
Member

I changed it to respect both. The rules were inconsistent as to which they looked at.

@jamesbraza
Copy link
Contributor Author

Thank you! Appreciate the fast turnaround 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants