NeMo icon indicating copy to clipboard operation
NeMo copied to clipboard

Unused imports cleanup

Open janekl opened this issue 1 year ago • 6 comments

What does this PR do ?

Unused imports cleanup indentified by Flake8, see https://www.flake8rules.com/rules/F401.html.

Collection: [ALL]

Jenkins CI

To run Jenkins, a NeMo User with write access must comment jenkins on the PR.

Before your PR is "Ready for review"

Pre checks:

  • [ ] Make sure you read and followed Contributor guidelines
  • [ ] Did you write any new necessary tests?
  • [ ] Did you add or update any necessary documentation?
  • [ ] Does the PR affect components that are optional to install? (Ex: Numba, Pynini, Apex etc)
    • [ ] Reviewer: Does the PR have correct import guards for all optional libraries?

PR Type:

  • [ ] New Feature
  • [ ] Bugfix
  • [ ] Documentation

If you haven't finished some of the above items you can still open "Draft" PR.

Who can review?

Anyone in the community is free to review the PR once the checks have passed. Contributor guidelines contains specific people who can review PRs to various areas.

Additional Information

  • Related to # (issue)

janekl avatar Feb 06 '24 15:02 janekl

jenkins

ericharper avatar Feb 06 '24 17:02 ericharper

There are also many odd choices, many torch.nn imports have been removed and arbitrarily using the longer torch.nn.xyz - why ?

titu1994 avatar Feb 06 '24 18:02 titu1994

There are also many odd choices, many torch.nn imports have been removed and arbitrarily using the longer torch.nn.xyz - why ?

The suggestions come from Flake8 tool, I just ran: flake8 . | grep F401 | grep -v "_init__.py", manually reviewed and applied them. So these are in fact just unused imports.

The only case I changed something is type annotation here https://github.com/NVIDIA/NeMo/blob/a0456c6ce1ac0731c0cec2acd3047c6ecb72e75d/examples/nlp/language_modeling/megatron_change_num_partitions.py#L1220

I can revert that if you prefer.

janekl avatar Feb 07 '24 11:02 janekl

This will create merge conflicts in practically every open pr. Why is this PR necessary ? If it's aesthetics, it's not sufficient reason to merge this.

@artbataev has the same issue with black, but has found a way to make it iterative and only pr level change where than global change that affects all open prs

  • I think git should be able handle conflicts automatically, these are typically one-line changes. They are effectively no-ops, and Jenkins CI passes. Yes, that's mostly for aesthetics -- I discussed with Eric that it's worth it
  • I would recommend looking into full Flake8 output besides, it discovers many issues besides (including undefined variables) so this is a low-hanging-fruit improvement

janekl avatar Feb 07 '24 11:02 janekl

The CI passes because it's testing this branch without any conflicts. Try manually rebasing a copy if any large open pr with your branch here and see if Pycharm can handle it automatically. Vlad and I have tried it, it could not handle blacks "one line changes" automatically either

titu1994 avatar Feb 07 '24 21:02 titu1994

If Pycharm can rebase and handle this PR with other large pr without requiring manual review, then it's fine. Otherwise it's not worth introducing merge conflicts across 80 open prs for an aesthetic change

titu1994 avatar Feb 07 '24 21:02 titu1994

This PR is stale because it has been open for 14 days with no activity. Remove stale label or comment or update or this will be closed in 7 days.

github-actions[bot] avatar Feb 22 '24 01:02 github-actions[bot]

This PR was closed because it has been inactive for 7 days since being marked as stale.

github-actions[bot] avatar Feb 29 '24 01:02 github-actions[bot]