test: cover converter CLI entry points#2387
Open
lonexreb wants to merge 1 commit intoNVIDIA-NeMo:mainfrom
Open
test: cover converter CLI entry points#2387lonexreb wants to merge 1 commit intoNVIDIA-NeMo:mainfrom
lonexreb wants to merge 1 commit intoNVIDIA-NeMo:mainfrom
Conversation
The existing functional roundtrip test only exercised the inner library
functions (convert_dcp_to_hf, export_model_from_megatron, merge_lora_to_hf
and export_lora_adapter_to_hf). The argparse + main() surfaces of the
three converter scripts under examples/converters/ were never invoked,
so regressions in their CLI contracts (missing flags, broken
config-vs-CLI overrides, wrong dispatch on --adapter-only, etc.) shipped
silently.
Closes the gap on two fronts:
1. New fast unit tests under tests/unit/converters/ exercise parse_args()
and main()'s argument-handling logic for all three scripts. The
underlying library calls are mocked out so the tests run without GPU,
Ray, or model downloads:
- test_convert_dcp_to_hf_args.py: validates flags, defaults, the
local-tokenizer-vs-config-fallback branch, and hf_overrides
forwarding (including the YAML-null-collapses-to-empty-dict edge
case).
- test_convert_megatron_to_hf_args.py (mcore-marked): validates flags,
--no-strict toggle wiring through to the strict= kwarg, and the
--hf-model-name override path.
- test_convert_lora_to_hf_args.py: validates required-flag
enforcement, --adapter-only dispatch in both branches, and the
refuse-to-overwrite contract on existing output paths.
2. Extended tests/functional/test_converter_roundtrip.py with a STEP 10
that invokes each script's main() in-process via sys.argv patching
against the same model artifacts produced earlier in the test, then
compares the resulting HF checkpoints against the previously
validated state dicts. This exercises the real end-to-end CLI path
for convert_dcp_to_hf.py, convert_megatron_to_hf.py, and both
branches of convert_lora_to_hf.py.
Refs: NVIDIA-NeMo#2259
Signed-off-by: lonexreb <reach2shubhankar@gmail.com>
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
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.
Summary
Closes the test-coverage gap identified in #2259 — the CLI entry points
(
parse_args+main) of the three converter scripts underexamples/converters/are now exercised by tests, both quickly viaunit tests and end-to-end against real model artifacts in the existing
functional roundtrip test.
The existing
tests/functional/test_converter_roundtrip.pyonly calledthe underlying library functions (
convert_dcp_to_hf,export_model_from_megatron,merge_lora_to_hf,export_lora_adapter_to_hf). The argparse +main()shells ofconvert_dcp_to_hf.py,convert_megatron_to_hf.py, andconvert_lora_to_hf.pywere never invoked, so regressions in their CLIcontracts (missing flags, broken config-vs-CLI overrides, wrong dispatch
on
--adapter-only, etc.) shipped silently.What's covered
New fast unit tests under
tests/unit/converters/These run without GPU, Ray, or model downloads. The underlying library
calls are mocked out so the tests stay in the L0 budget.
test_convert_dcp_to_hf_args.py— flags, defaults, unknown-flagexit code, plus
main()'s local-tokenizer-vs-config-fallback branchselection and
hf_overridesforwarding (including the YAMLnull → {}collapse).test_convert_megatron_to_hf_args.py(markedmcore) — flags,defaults, the
--no-stricttoggle wiring through to thestrict=kwarg, and the
--hf-model-nameoverride path.test_convert_lora_to_hf_args.py— required-flag enforcement(parametrized over each),
--adapter-onlydispatch in bothbranches, and the refuse-to-overwrite contract on existing output
paths.
Extended functional test (
tests/functional/test_converter_roundtrip.py)Added a STEP 10 that invokes each script's
main()in-process viasys.argvpatching, against the same model artifacts produced earlierin the test, and compares the resulting HF checkpoints against the
previously validated state dicts:
convert_dcp_to_hf.pyCLI on the v1 DCP checkpoint (alsoexercises the local-tokenizer-path branch by saving the tokenizer
next to the DCP ckpt).
convert_megatron_to_hf.pyCLI on the Megatron checkpoint.convert_lora_to_hf.pyCLI — merged-model branch (no--adapter-only).convert_lora_to_hf.pyCLI — adapter-only branch(
--adapter-only), verified by checking the resulting PEFTdirectory layout.
Test plan
ruff check tests/unit/converters/ tests/functional/test_converter_roundtrip.py— cleanruff format— appliedpython3 -m py_compile— cleantests/unit/converters/suite--mcore-only(CI) — runstest_convert_megatron_to_hf_args.pytests/functional/test_converters.sh(CI) — runs the extendedroundtrip including STEP 10
Notes
repo convention.
main()-exercising unit test uses a freshimportlib-loadedmodule copy so
sys.argvpatches cannot leak across tests._run_script_maininvokesmain()in-process to keep STEP 10 inside the same Ray/CUDA context as the
rest of the test.
Refs: #2259