test(converters): add CLI entry-point coverage for all three converte…#2666
Open
SakethKoona wants to merge 1 commit into
Open
test(converters): add CLI entry-point coverage for all three converte…#2666SakethKoona wants to merge 1 commit into
SakethKoona wants to merge 1 commit into
Conversation
…r scripts Extract shared helpers from test_converter_roundtrip.py into converter_test_utils.py so the setup and verify scripts used by the shell-driven CLI tests can reuse checkpoint creation and assertion logic without duplication. Add _cli_test_setup.py, _cli_test_verify_dcp.py, _cli_test_verify_meg.py, and _cli_test_verify_lora_merge.py to drive and verify each converter CLI end-to-end. Expand test_converters.sh with a second block that invokes all three converter scripts (five invocations total) via subprocess, covering parse_args()+main() paths that were previously untested: config.yaml reading, tokenizer fallback branch, --hf-model-name override, --adapter-only dispatch, and hf_overrides extraction. Signed-off-by: Saketh <koona.saketh@gmail.com>
svcnvidia-nemo-ci
added
the
waiting-on-maintainers
6 tasks
yuki-97
reviewed
Jun 6, 2026
yuki-97
left a comment
yuki-97
left a comment
Contributor
There was a problem hiding this comment.
@SakethKoona thanks so much for supporting this! overall LGTM and left some minor comments.
Contributor
There was a problem hiding this comment.
maybe rename to _converter_test_setup to make it clear?
Contributor
There was a problem hiding this comment.
do you mind merging the three verify files to one _converter_test_verify.py, and use input args to control what to verify?
Contributor
There was a problem hiding this comment.
nit: what about rename to _converter_test_utils.py?
| write_config_yaml, | ||
| ) | ||
|
|
||
| tmp, model = sys.argv[1], sys.argv[2] |
Contributor
There was a problem hiding this comment.
nit:
Suggested change
| tmp, model = sys.argv[1], sys.argv[2] | |
| tmp_path, model = sys.argv[1], sys.argv[2] |
|
|
||
| # create_megatron_checkpoint returns the iter_0000000 subdirectory path, | ||
| # which is what --megatron-ckpt-path and --base-ckpt both expect. | ||
| meg_path = create_megatron_checkpoint(model, tmp) |
Contributor
There was a problem hiding this comment.
nit: wdyt changing all the meg to megatron?
Suggested change
| meg_path = create_megatron_checkpoint(model, tmp) | |
| megatron_path = create_megatron_checkpoint(model, tmp) |
|
|
||
| tmp, model = sys.argv[1], sys.argv[2] | ||
|
|
||
| config = create_test_config() |
Contributor
There was a problem hiding this comment.
we've hardcode model_name in create_test_config, could you make it a input args instead of hardcode?
so that the script is still fine if we change test model in tests/functional/test_converters.sh.
Suggested change
| config = create_test_config() | |
| config = create_test_config(model) |
svcnvidia-nemo-ci
added
waiting-on-customer
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
test_converter_roundtrip.py previously tested the converter library functions directly but never exercised the parse_args() + main() entry points of the three scripts in examples/converters/. This PR adds shell-driven CLI tests that invoke each script as a subprocess (exactly as a user would), covering logic branches that were entirely untested: config.yaml reading, tokenizer path fallback, --hf-model-name override, --adapter-only dispatch, and hf_overrides extraction.
Issues
Addresses #2259
Changes
checkpoint-creation and assertion logic between the existing roundtrip test and the new CLI tests.
Pre checks
Additional Information
test_converters.sh now runs two blocks back-to-back. Block 1 is the existing library-level roundtrip test; Block 2 is the new CLI entry-point tests. Both require at least one GPU. The two
blocks create independent checkpoints in separate temp directories and clean up after themselves via trap cleanup EXIT.