Skip to content

Add BERT/Qwen2.5 Unit test and Refactor all GHA Conversion Tests #12785

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 30 commits into from
Mar 31, 2025
Merged

Add BERT/Qwen2.5 Unit test and Refactor all GHA Conversion Tests #12785

merged 30 commits into from
Mar 31, 2025

Conversation

suiyoubi
Copy link
Collaborator

Important

The Update branch button must only be pressed in very rare occassions.
An outdated branch is never blocking the merge of a PR.
Please reach out to the automation team before pressing that button.

What does this PR do ?

Add a one line overview of what this PR aims to accomplish.

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)

github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 26, 2025
View reviewed changes
@suiyoubi
Add BERT CICD Tests
ab5c698 
Signed-off-by: Ao Tang <[email protected]>
@github-actions github-actions bot added the CI label Mar 26, 2025
ko3n1g
ko3n1g requested changes Mar 26, 2025
View reviewed changes
.github/workflows/cicd-main.yml Outdated
with:
RUNNER: self-hosted-azure
SCRIPT: |-
RUN_ID=${{ github.run_id }} BERT_TYPE=megatron bash tests/functional_tests/L2_NeMo_2_BERT_Pretraining.sh
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you refactor this to one script per test? We'd like to see fully self contained scripts so that we can move these tests between different CI systems easily.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, does this apply to all conversion test as well ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I noticed this too late unfortunately. It would be amazing if you could help with that refactoring too 🙏

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok sounds good.I'll refactor them in this pr

github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 26, 2025
View reviewed changes
suiyoubi and others added 9 commits March 26, 2025 10:45
@suiyoubi
Add test for Qwen25
dab51e4 
Signed-off-by: Ao Tang <[email protected]>
@suiyoubi
Apply isort and black reformatting
14901b0 
Signed-off-by: suiyoubi <[email protected]>
@suiyoubi @github-advanced-security
Potential fix for code scanning alert no. 13628: Unused local variable
fdd5043 
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Signed-off-by: Ao Tang <[email protected]>
@suiyoubi @github-advanced-security
Potential fix for code scanning alert no. 13635: Unused import
6106cb3 
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Signed-off-by: Ao Tang <[email protected]>
@suiyoubi @github-advanced-security
Potential fix for code scanning alert no. 13641: Unused import
ef65fec 
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Signed-off-by: Ao Tang <[email protected]>
@suiyoubi
Separate test for bert pretraining
bf6fd16 
Signed-off-by: Ao Tang <[email protected]>
@suiyoubi
pylint
8e9fa76 
Signed-off-by: Ao Tang <[email protected]>
@suiyoubi
Fix broken tests
49e9851 
Signed-off-by: Ao Tang <[email protected]>
@suiyoubi
Refactor all conversion tests
d88b6de 
Signed-off-by: Ao Tang <[email protected]>
@suiyoubi suiyoubi changed the title Add BERT Unit test Add BERT/Qwen2.5 Unit test and Refactor all GHA Conversion Tests Mar 26, 2025
@suiyoubi
rename
7d560e1 
Signed-off-by: Ao Tang <[email protected]>
@ko3n1g ko3n1g removed the Run CICD label Mar 26, 2025
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 29, 2025
View reviewed changes
tests/collections/llm/bert/model/test_bert_spec.py
Comment on lines +20 to +24
from nemo.collections.llm.bert.model.bert_spec import (
TransformerLayerWithPostLNSupport,
get_bert_layer_local_spec_postln,
get_bert_layer_with_transformer_engine_spec_postln,
)

Check notice

Code scanning / CodeQL

Unused import Note test

Import of 'TransformerLayerWithPostLNSupport' is not used.
Import of 'get_bert_layer_local_spec_postln' is not used.
Import of 'get_bert_layer_with_transformer_engine_spec_postln' is not used.

Copilot Autofix

AI 3 months ago

To fix the problem, we need to remove the unused import statement for TransformerLayerWithPostLNSupport. This will clean up the code and remove an unnecessary dependency. The best way to fix this is to edit the import statement on line 20 to exclude TransformerLayerWithPostLNSupport.

Suggested changeset 1
tests/collections/llm/bert/model/test_bert_spec.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/tests/collections/llm/bert/model/test_bert_spec.py b/tests/collections/llm/bert/model/test_bert_spec.py
--- a/tests/collections/llm/bert/model/test_bert_spec.py
+++ b/tests/collections/llm/bert/model/test_bert_spec.py
@@ -20,3 +20,2 @@
 from nemo.collections.llm.bert.model.bert_spec import (
-    TransformerLayerWithPostLNSupport,
     get_bert_layer_local_spec_postln,
EOF
@@ -20,3 +20,2 @@
from nemo.collections.llm.bert.model.bert_spec import (
TransformerLayerWithPostLNSupport,
get_bert_layer_local_spec_postln,
Copilot is powered by AI and may make mistakes. Always verify output.
tests/collections/llm/bert_pretraining.py
# See the License for the specific language governing permissions and
# limitations under the License.
import argparse
import os

Check notice

Code scanning / CodeQL

Unused import Note test

Import of 'os' is not used.

Copilot Autofix

AI 3 months ago

To fix the problem, we need to remove the unused import statement for the os module. This will clean up the code and remove the unnecessary dependency.

  • Locate the import statement for the os module.
  • Remove the line that imports the os module.
  • Ensure that no other parts of the code are affected by this change.
Suggested changeset 1
tests/collections/llm/bert_pretraining.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/tests/collections/llm/bert_pretraining.py b/tests/collections/llm/bert_pretraining.py
--- a/tests/collections/llm/bert_pretraining.py
+++ b/tests/collections/llm/bert_pretraining.py
@@ -14,3 +14,2 @@
 import argparse
-import os
 from dataclasses import dataclass
EOF
@@ -14,3 +14,2 @@
import argparse
import os
from dataclasses import dataclass
Copilot is powered by AI and may make mistakes. Always verify output.
tests/collections/llm/bert_pretraining.py
# limitations under the License.
import argparse
import os
from dataclasses import dataclass

Check notice

Code scanning / CodeQL

Unused import Note test

Import of 'dataclass' is not used.

Copilot Autofix

AI 3 months ago

To fix the problem, we need to remove the unused import statement. This will clean up the code and eliminate the unnecessary dependency. The best way to fix this issue is to delete the line that imports dataclass from the dataclasses module.

Suggested changeset 1
tests/collections/llm/bert_pretraining.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/tests/collections/llm/bert_pretraining.py b/tests/collections/llm/bert_pretraining.py
--- a/tests/collections/llm/bert_pretraining.py
+++ b/tests/collections/llm/bert_pretraining.py
@@ -15,3 +15,2 @@
 import os
-from dataclasses import dataclass
 
EOF
@@ -15,3 +15,2 @@
import os
from dataclasses import dataclass

Copilot is powered by AI and may make mistakes. Always verify output.
@suiyoubi
Fix Gemma2 file name
b5c7641 
Signed-off-by: Ao Tang <[email protected]>
@ko3n1g ko3n1g added Run CICD and removed Run CICD labels Mar 30, 2025
suiyoubi added 2 commits March 29, 2025 22:20
@suiyoubi
remove --branch
5346a07 
Signed-off-by: Ao Tang <[email protected]>
@suiyoubi
Merge remote-tracking branch 'origin/main' into aot/bert-unit-test
e34b928 
# Conflicts:
#	tests/functional_tests/L2_NeMo_2_Conversion_Test.sh
@ko3n1g ko3n1g added Run CICD and removed Run CICD labels Mar 30, 2025
@suiyoubi
Fix evo2 test
8956592 
Signed-off-by: Ao Tang <[email protected]>
@ko3n1g ko3n1g added Run CICD and removed Run CICD labels Mar 30, 2025
@github-actions github-actions bot removed the Run CICD label Mar 30, 2025
@github-actions GitHub Actions
Copy link
Contributor

[🤖]: Hi @suiyoubi 👋,

We wanted to let you know that a CICD pipeline for this PR just finished successfully

So it might be time to merge this PR or get some approvals

I'm just a bot so I'll leave it you what to do next.

//cc @pablo-garay @ko3n1g

@ko3n1g ko3n1g enabled auto-merge (squash) March 30, 2025 20:59
@github-actions github-actions bot removed the Run CICD label Mar 31, 2025
@github-actions GitHub Actions
Copy link
Contributor

[🤖]: Hi @suiyoubi 👋,

We wanted to let you know that a CICD pipeline for this PR just finished successfully

So it might be time to merge this PR or get some approvals

I'm just a bot so I'll leave it you what to do next.

//cc @pablo-garay @ko3n1g

@ko3n1g ko3n1g merged commit dc8c441 into main Mar 31, 2025
229 of 230 checks passed
@ko3n1g ko3n1g deleted the aot/bert-unit-test branch March 31, 2025 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ko3n1g ko3n1g ko3n1g approved these changes

@pablo-garay pablo-garay Awaiting requested review from pablo-garay pablo-garay is a code owner

@thomasdhc thomasdhc Awaiting requested review from thomasdhc thomasdhc is a code owner

@chtruong814 chtruong814 Awaiting requested review from chtruong814 chtruong814 is a code owner

Assignees
No one assigned
Labels
CI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@suiyoubi @ko3n1g