[recipe, test] feat: add unit tests for CometPlugin#3652
Open
lonexreb wants to merge 2 commits intoNVIDIA-NeMo:mainfrom
Open
[recipe, test] feat: add unit tests for CometPlugin#3652lonexreb wants to merge 2 commits intoNVIDIA-NeMo:mainfrom
lonexreb wants to merge 2 commits intoNVIDIA-NeMo:mainfrom
Conversation
`run_plugins.py` exports a `CometPlugin` for wiring Comet ML logging
through a `run.Script` task, but `test_run_plugins.py` had test classes
for every other plugin (Wandb, Nsys, Preemption, FaultTolerance,
PyTorchProfiler, PerfEnv) except CometPlugin.
Add `TestCometPlugin` mirroring the `TestWandbPlugin` pattern. 7 new
tests cover:
- field initialization (project, name, workspace) including the
optional-field default-to-None case
- happy-path setup with COMET_API_KEY: env var forwarded to
executor.env_vars and the documented Hydra-style CLI overrides
(`logger.comet_project`, `logger.comet_workspace`,
`logger.comet_experiment_name`) appended to task.args
- bare-project-only setup: when name and workspace are None, only
the project override is emitted (the others are NOT)
- missing COMET_API_KEY: a clear warning is logged, no env var is
forwarded, and no CLI overrides are appended
- non-Script task path: NotImplementedError with the documented
message, AND the API key is still forwarded to the executor
(current contract — exposing this in a test means a future
refactor that drops it will fail loudly)
- custom `script_args_converter_fn`: replaces the default
Hydra-style overrides entirely (a new `--comet-*` style is emitted,
Hydra-style is NOT)
Tests-only — no production code changes.
Signed-off-by: lonexreb <[email protected]>
Contributor
|
/ok to test b4c8d31 |
cuichenx
added
the
ready-to-merge
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.
Summary
run_plugins.pyexports aCometPluginfor wiring Comet ML logging through arun.Scripttask, buttest_run_plugins.pyhad test classes for every other plugin in the file (Wandb, Nsys, Preemption, FaultTolerance, PyTorchProfiler, PerfEnv) — except CometPlugin.This PR adds
TestCometPlugin(7 tests, +158 LoC) mirroring theTestWandbPluginpattern. Tests-only — no production code changes.What's covered
test_initializationtest_initialization_optional_fields_default_to_noneNonetest_setup_with_script_taskCOMET_API_KEYforwarded toexecutor.env_vars; documented Hydra overrides (logger.comet_project,logger.comet_workspace,logger.comet_experiment_name) appended totask.argstest_setup_with_only_required_fieldname/workspaceareNone, only the project override is emitted — the others are NOTtest_setup_without_api_key_warns_and_does_not_propagateCOMET_API_KEY⇒ clear warning, no env var, no CLI overridestest_setup_raises_for_non_script_taskrun.Partialtask ⇒NotImplementedError. AND the API key is still forwarded BEFORE the type check (current contract — locking this in means a future refactor that drops it fails loudly)test_custom_script_args_converterscript_args_converter_fnreplaces the default Hydra overrides entirelyTest plan
python3 -m astparse cleanruff checkcleanruff format --checkcleancicd-unit-tests-corepicks up the new test class automatically (file already covered)Risk
Zero — tests only.