ci: Run linting per domain by ko3n1g · Pull Request #12027 · NVIDIA-NeMo/NeMo

ko3n1g · 2025-02-03T10:07:21Z

What does this PR do ?

This PR changes the way we run linting.

Moving forward, we run different linting sets per domain. So far, we have the domains speech and others. Others can be split into more fine-grained domains on demand.
No PR comments anymore, summaries are writing into the check summary (example: https://github.com/NVIDIA/NeMo/actions/runs/13113871995#summary-36583490219)
Each domain can configure their own pylint and flake8 rules.
We use path-filters to run domain-rules on files accordingly
A PR can only get merged if all domain-rules are satisfied

QnA:

How to change domain rules? -> Get in touch with domain PiC
I am a PiC but my domain isn’t represented yet -> Get in touch with automation team
I am a PiC and would like to change domain rules: Modify .pylintrc.$DOMAIN or .flake8.$DOMAIN to your needs
Something doesn't work right: Get in touch with the automation team

Collection: [Note which collection this PR will affect]

Changelog

Add specific line by line info of high level changes in this PR.

Usage

You can potentially add a usage example below

# Add a code snippet demonstrating how to use this

GitHub Actions CI

The Jenkins CI system has been replaced by GitHub Actions self-hosted runners.

The GitHub Actions CI will run automatically when the "Run CICD" label is added to the PR.
To re-run CI remove and add the label again.
To run CI on an untrusted fork, a NeMo user with write access must first click "Approve and run".

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)

nemo/collections/asr/data/audio_to_diar_label_lhotse.py

Signed-off-by: oliver könig <okoenig@nvidia.com>

nemo/core/config/base_config.py

pzelasko · 2025-02-03T15:32:58Z

.github/workflows/code-linting.yml

+            FILTER=$(jq -crn '[
+              "nemo/collections/asr/**",
+              "nemo/collections/tts/**",
+              "nemo/collections/audio/**"


Can we extend this with nemo/collections/multimodal/speech_llm (nemo 1 speech llm) and nemo/collections/speechlm (nemo 2 speech llm)?

pzelasko · 2025-02-03T15:33:52Z

nemo/collections/asr/data/audio_to_diar_label_lhotse.py

 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
-# limitations under the License.
+# limitations under the License..


nitpick: why the extra full stop here?

this is just to trigger a test run:)

pzelasko

Let's extend speech rules to speech llm as I indicated in the other comment and LGTM

Signed-off-by: oliver könig <okoenig@nvidia.com>

github-actions · 2025-02-03T16:11:06Z

beep boop 🤖: 🚨 The following files must be fixed before merge!

Your code was analyzed with PyLint. The following annotations have been identified:

************* Module nemo.collections.asr.data.audio_to_diar_label_lhotse
nemo/collections/asr/data/audio_to_diar_label_lhotse.py:1:0: C0114: Missing module docstring (missing-module-docstring)
nemo/collections/asr/data/audio_to_diar_label_lhotse.py:17:0: E0401: Unable to import 'torch.utils.data' (import-error)
nemo/collections/asr/data/audio_to_diar_label_lhotse.py:18:0: E0401: Unable to import 'lhotse.dataset' (import-error)
nemo/collections/asr/data/audio_to_diar_label_lhotse.py:19:0: E0401: Unable to import 'lhotse.dataset.collation' (import-error)
nemo/collections/asr/data/audio_to_diar_label_lhotse.py:28:0: C0115: Missing class docstring (missing-class-docstring)
nemo/collections/asr/data/audio_to_diar_label_lhotse.py:15:0: W0611: Unused List imported from typing (unused-import)

-----------------------------------
Your code has been rated at 4.00/10

Mitigation guide:

Add sensible and useful docstrings to functions and methods
For trivial methods like getter/setters, consider adding # pylint: disable=C0116 inside the function itself
To disable multiple functions/methods at once, put a # pylint: disable=C0116 before the first and a # pylint: enable=C0116 after the last.

By applying these rules, we reduce the occurance of this message in future.

Thank you for improving NeMo's documentation!

github-actions · 2025-02-03T16:11:09Z

beep boop 🤖: 🙏 The following files have warnings. In case you are familiar with these, please try helping us to improve the code base.

Your code was analyzed with PyLint. The following annotations have been identified:

************* Module nemo.collections.asr.data.audio_to_ctm_dataset
nemo/collections/asr/data/audio_to_ctm_dataset.py:47:0: C0301: Line too long (132/100) (line-too-long)
nemo/collections/asr/data/audio_to_ctm_dataset.py:78:0: C0301: Line too long (113/100) (line-too-long)
nemo/collections/asr/data/audio_to_ctm_dataset.py:78:0: C0301: Line too long (113/100) (line-too-long)
nemo/collections/asr/data/audio_to_ctm_dataset.py:1:0: C0114: Missing module docstring (missing-module-docstring)
nemo/collections/asr/data/audio_to_ctm_dataset.py:38:4: C0116: Missing function or method docstring (missing-function-docstring)
nemo/collections/asr/data/audio_to_ctm_dataset.py:50:0: C0115: Missing class docstring (missing-class-docstring)
nemo/collections/asr/data/audio_to_ctm_dataset.py:57:4: C0116: Missing function or method docstring (missing-function-docstring)
nemo/collections/asr/data/audio_to_ctm_dataset.py:57:40: C0103: Argument name "frameCtmUnits" doesn't conform to snake_case naming style (invalid-name)
nemo/collections/asr/data/audio_to_ctm_dataset.py:62:4: R0913: Too many arguments (8/5) (too-many-arguments)
nemo/collections/asr/data/audio_to_ctm_dataset.py:62:4: R0917: Too many positional arguments (8/5) (too-many-positional-arguments)
nemo/collections/asr/data/audio_to_ctm_dataset.py:62:4: R1711: Useless return at end of function or method (useless-return)
************* Module nemo.core.config.base_config
nemo/core/config/base_config.py:1:0: C0114: Missing module docstring (missing-module-docstring)
nemo/core/config/base_config.py:16:0: W0611: Unused List imported from typing (unused-import)

-----------------------------------
Your code has been rated at 7.55/10

Mitigation guide:

Add sensible and useful docstrings to functions and methods
For trivial methods like getter/setters, consider adding # pylint: disable=C0116 inside the function itself
To disable multiple functions/methods at once, put a # pylint: disable=C0116 before the first and a # pylint: enable=C0116 after the last.

By applying these rules, we reduce the occurance of this message in future.

Thank you for improving NeMo's documentation!

titu1994

Thanks ! Maybe we can refactor the list of bypassed domains into a file and read that so there's one central location, but I guess it's not that easy and not really necessary either for now

* ci: Run linting per domain Signed-off-by: oliver könig <okoenig@nvidia.com> * add codeowners Signed-off-by: oliver könig <okoenig@nvidia.com> * add speechllm Signed-off-by: oliver könig <okoenig@nvidia.com> --------- Signed-off-by: oliver könig <okoenig@nvidia.com>

* ci: Run linting per domain Signed-off-by: oliver könig <okoenig@nvidia.com> * add codeowners Signed-off-by: oliver könig <okoenig@nvidia.com> * add speechllm Signed-off-by: oliver könig <okoenig@nvidia.com> --------- Signed-off-by: oliver könig <okoenig@nvidia.com> Signed-off-by: Youngeun Kwon <youngeunk@nvidia.com>

github-actions bot added CI ASR labels Feb 3, 2025

ko3n1g changed the title ~~ci: Run linting per domai~~ ci: Run linting per domain Feb 3, 2025

ko3n1g force-pushed the ko3n1g/ci/linting branch from b3a37dd to 9f37b2b Compare February 3, 2025 10:52

github-advanced-security bot found potential problems Feb 3, 2025

View reviewed changes

nemo/collections/asr/data/audio_to_diar_label_lhotse.py Fixed Show fixed Hide fixed

github-actions bot added the core Changes to NeMo Core label Feb 3, 2025

ci: Run linting per domain

2be845e

Signed-off-by: oliver könig <okoenig@nvidia.com>

ko3n1g force-pushed the ko3n1g/ci/linting branch from 6687e0c to 9108036 Compare February 3, 2025 12:45

ko3n1g requested review from pzelasko and titu1994 February 3, 2025 12:45

ko3n1g marked this pull request as ready for review February 3, 2025 12:50

ko3n1g requested review from chtruong814, pablo-garay and thomasdhc as code owners February 3, 2025 12:50

ko3n1g requested a review from ericharper February 3, 2025 12:52

github-advanced-security bot found potential problems Feb 3, 2025

View reviewed changes

nemo/core/config/base_config.py Fixed Show fixed Hide fixed

pzelasko reviewed Feb 3, 2025

View reviewed changes

pzelasko requested changes Feb 3, 2025

View reviewed changes

ko3n1g added 2 commits February 3, 2025 17:10

add codeowners

e3214eb

Signed-off-by: oliver könig <okoenig@nvidia.com>

add speechllm

7f2dcc3

Signed-off-by: oliver könig <okoenig@nvidia.com>

ko3n1g force-pushed the ko3n1g/ci/linting branch from c587778 to 7f2dcc3 Compare February 3, 2025 16:11

github-actions bot removed core Changes to NeMo Core ASR labels Feb 3, 2025

ko3n1g requested a review from pzelasko February 3, 2025 16:13

titu1994 approved these changes Feb 3, 2025

View reviewed changes

ko3n1g merged commit 2f035d3 into main Feb 3, 2025
20 checks passed

ko3n1g deleted the ko3n1g/ci/linting branch February 3, 2025 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: Run linting per domain#12027

ci: Run linting per domain#12027
ko3n1g merged 3 commits intomainfrom
ko3n1g/ci/linting

ko3n1g commented Feb 3, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

pzelasko Feb 3, 2025

Uh oh!

ko3n1g Feb 3, 2025

Uh oh!

pzelasko Feb 3, 2025

Uh oh!

ko3n1g Feb 3, 2025

Uh oh!

pzelasko left a comment

Uh oh!

github-actions bot commented Feb 3, 2025

Uh oh!

github-actions bot commented Feb 3, 2025

Uh oh!

titu1994 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

ko3n1g commented Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do ?

Changelog

Usage

GitHub Actions CI

Before your PR is "Ready for review"

Who can review?

Additional Information

Uh oh!

Uh oh!

Uh oh!

pzelasko Feb 3, 2025

Choose a reason for hiding this comment

Uh oh!

ko3n1g Feb 3, 2025

Choose a reason for hiding this comment

Uh oh!

pzelasko Feb 3, 2025

Choose a reason for hiding this comment

Uh oh!

ko3n1g Feb 3, 2025

Choose a reason for hiding this comment

Uh oh!

pzelasko left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 3, 2025

Uh oh!

github-actions bot commented Feb 3, 2025

Uh oh!

titu1994 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ko3n1g commented Feb 3, 2025 •

edited

Loading