NeMo
NeMo copied to clipboard
Unused imports cleanup
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)
jenkins
There are also many odd choices, many torch.nn imports have been removed and arbitrarily using the longer torch.nn.xyz - why ?
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.
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
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
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
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.
This PR was closed because it has been inactive for 7 days since being marked as stale.