[tests] test: add CPU coverage for MixGRPO and GRPO-Guard#123
[tests] test: add CPU coverage for MixGRPO and GRPO-Guard#123SamitHuang wants to merge 1 commit into
Conversation
Add cheap CPU unit tests for MixGRPO window positioning/registry dispatch and extend GRPO-Guard coverage (loss registry, config validation, rollout correction integration) without expensive end-to-end runs. Co-authored-by: GitHub Copilot
There was a problem hiding this comment.
Code Review
This pull request introduces CPU tests for the MixGRPO pipeline registration and SDE window positioning, registers and tests the new grpo_guard loss mode, and adds integration tests for the grpo_guard loss. The feedback suggests standardizing the instantiation of DiffusionModelConfig to avoid bypassing validation, increasing the range in the random window positioning test to prevent flaky test failures, and resolving the configuration directory path relative to the package directory to make the test suite more robust.
| def _make_model_config(algorithm: str = "mix_grpo") -> DiffusionModelConfig: | ||
| cfg = object.__new__(DiffusionModelConfig) | ||
| object.__setattr__(cfg, "architecture", "QwenImagePipeline") | ||
| object.__setattr__(cfg, "algorithm", algorithm) | ||
| object.__setattr__(cfg, "external_lib", None) | ||
| return cfg |
There was a problem hiding this comment.
Using object.__new__ and object.__setattr__ to bypass the standard constructor of DiffusionModelConfig is fragile and bypasses validation, default values, and type checks. If DiffusionModelConfig is a standard dataclass, it should be instantiated normally to ensure maintainability and correctness.
| def _make_model_config(algorithm: str = "mix_grpo") -> DiffusionModelConfig: | |
| cfg = object.__new__(DiffusionModelConfig) | |
| object.__setattr__(cfg, "architecture", "QwenImagePipeline") | |
| object.__setattr__(cfg, "algorithm", algorithm) | |
| object.__setattr__(cfg, "external_lib", None) | |
| return cfg | |
| def _make_model_config(algorithm: str = "mix_grpo") -> DiffusionModelConfig: | |
| return DiffusionModelConfig( | |
| architecture="QwenImagePipeline", | |
| algorithm=algorithm, | |
| external_lib=None, | |
| ) |
References
- Adhere to standard object instantiation and PEP 8 best practices to avoid bypassing class constructors and type-safety mechanisms. (link)
| "sample_strategy": "random", | ||
| "sde_window_seed": 99, | ||
| "sde_window_size": 2, | ||
| "sde_window_range": [0, 10], |
There was a problem hiding this comment.
The current test uses a small sde_window_range of [0, 10]. With sde_window_size = 2, the maximum start index is 8, meaning there are only 9 possible start positions. Since the test compares two random draws with different seeds (global_steps = 0 vs global_steps = 1), there is a 1 in 9 (~11.1%) chance that both seeds will randomly map to the same start position, causing the test to fail flakily.
Increasing the range to [0, 100000] reduces the collision probability to 1 in 100,000, making the test robust and deterministic.
| "sde_window_range": [0, 10], | |
| "sde_window_range": [0, 100000], |
| from hydra import compose, initialize_config_dir | ||
| from verl.utils.config import omega_conf_to_dataclass | ||
|
|
||
| config_dir = os.path.abspath("verl_omni/trainer/config/diffusion/actor") |
There was a problem hiding this comment.
Using os.path.abspath with a hardcoded relative path like "verl_omni/trainer/config/diffusion/actor" makes the test suite fragile because it depends on the current working directory from which pytest is executed. If pytest is run from a subdirectory (e.g., tests/), the path resolution will fail.
Instead, resolve the path relative to the verl_omni package directory using os.path.dirname(verl_omni.__file__), which is more robust and consistent with other tests in the codebase.
import verl_omni
config_dir = os.path.join(os.path.dirname(verl_omni.__file__), "trainer/config/diffusion/actor")There was a problem hiding this comment.
add mix_grpo test under tests/trainer/diffusion/test_diffusion_core_algos_on_cpu.py instead of creating a new file
What does this PR do?
Add cheap CPU unit tests for MixGRPO window positioning/registry dispatch and extend GRPO-Guard coverage (loss registry, config validation, rollout correction integration) without expensive end-to-end runs.
Checklist Before Starting
[{modules}] {type}: {description}(This will be checked by the CI){modules}includefsdp,vllm_omni,rollout,trainer,ci,training_utils,recipe,ray,worker,single_controller,misc,perf,model,algo,env,tool,ckpt,doc,data,cfg,reward,diffusion,omni,tests,docker,like[diffusion, doc]{type}is infeat,fix,refactor,chore,test[BREAKING]to the beginning of the title.[BREAKING][diffusion, fsdp] feat: new rollout schedulerTest
API and Usage Example
# Add code snippet or script demonstrating how to use thisDesign & Code Changes
Checklist Before Submitting
Important
Please check all the following items before requesting a review, otherwise the reviewer might deprioritize this PR for review.
pre-commit install && pre-commit run --all-files --show-diff-on-failure --color=always