fix: add mask seq with high logp err to nemo gym config - #1980
Conversation
Signed-off-by: cmunley1 <cmunley@nvidia.com>
📝 WalkthroughWalkthroughThis PR adds a new configuration field Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
examples/nemo_gym/grpo_workplace_assistant_nemotron_nano_v2_9b.yaml (1)
38-38: Add an inline comment describing the field's purpose, type, and valid range.Every non-obvious parameter in this file carries a comment (e.g.,
overlong_filtering,skip_reference_policy_logprobs_calculation,moe_router_load_balancing_type).seq_logprob_error_thresholdis opaque without one — readers have no indication of what unit (float, log-prob delta?), what range triggers masking, or a cross-reference to docs/guides.✏️ Suggested comment
- seq_logprob_error_threshold: null + seq_logprob_error_threshold: null # Mask sequences whose per-seq logprob error (vLLM vs MCore) exceeds this value; null disables masking. See docs/guides/grpo.md.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/nemo_gym/grpo_workplace_assistant_nemotron_nano_v2_9b.yaml` at line 38, Add an inline comment for the seq_logprob_error_threshold field explaining its purpose (what it controls—e.g., threshold for masking tokens based on sequence log-probability errors), its type (float), valid range (e.g., 0.0–inf or a recommended small positive range like 0.0–10.0 and whether negatives are allowed), the unit/meaning (log-prob delta or absolute log-prob), and a brief pointer to docs or a config section for more details; update the line containing seq_logprob_error_threshold to include this one-line comment referencing the field name so readers know how to set it and what values are safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@examples/nemo_gym/grpo_workplace_assistant_nemotron_nano_v2_9b.yaml`:
- Line 38: Add an inline comment for the seq_logprob_error_threshold field
explaining its purpose (what it controls—e.g., threshold for masking tokens
based on sequence log-probability errors), its type (float), valid range (e.g.,
0.0–inf or a recommended small positive range like 0.0–10.0 and whether
negatives are allowed), the unit/meaning (log-prob delta or absolute log-prob),
and a brief pointer to docs or a config section for more details; update the
line containing seq_logprob_error_threshold to include this one-line comment
referencing the field name so readers know how to set it and what values are
safe.
…1980) Signed-off-by: cmunley1 <cmunley@nvidia.com> Signed-off-by: yuanhangs <yuanhangs@nvidia.com>
…1980) Signed-off-by: cmunley1 <cmunley@nvidia.com>
Signed-off-by: cmunley1 <cmunley@nvidia.com>
Signed-off-by: cmunley1 <cmunley@nvidia.com>
Signed-off-by: cmunley1 <cmunley@nvidia.com>
What does this PR do ?
Add a one line overview of what this PR aims to accomplish.
Issues
List issues that this PR closes (syntax):
Usage
# Add a code snippet demonstrating how to use thisBefore your PR is "Ready for review"
Pre checks:
Additional Information
Summary by CodeRabbit