Conversation
… example (#2283) Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com> Signed-off-by: NeMo Bot <nemo-bot@nvidia.com>
|
/ok to test 11c6a55 |
📝 WalkthroughWalkthroughUpdates documentation examples in the callbacks guide to use new imports (time, forward_step, qwen25_500m_pretrain_config) and replace forward_step_func with forward_step, adding config instantiation across multiple example sections. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 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)
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/training/callbacks.md`:
- Around line 67-82: The example uses undefined callback classes (MyCallback,
TimingCallback, MetricsCallback) when creating CallbackManager and calling
manager.add/register; update the snippet to be self-contained by either
importing/defining the referenced callbacks or adding a short inline comment
above the manager.add calls stating these are placeholder/example callback names
that must be defined or imported by the user (e.g., "Assuming
MyCallback/TimingCallback/MetricsCallback are defined elsewhere"), and ensure
references to CallbackManager, manager.add, and manager.register remain correct.
| ```python | ||
| from megatron.bridge.training.callbacks import CallbackManager | ||
| from megatron.bridge.training.gpt_step import forward_step | ||
| from megatron.bridge.training.pretrain import pretrain | ||
| from megatron.bridge.recipes.qwen import qwen25_500m_pretrain_config | ||
|
|
||
| manager = CallbackManager() | ||
| manager.add(MyCallback()) | ||
| manager.add([TimingCallback(), MetricsCallback()]) | ||
| manager.register("on_eval_end", lambda ctx: print("Evaluation complete!")) | ||
|
|
||
| pretrain(config, forward_step_func, callbacks=manager) | ||
| # Create a config that fits on a single GPU | ||
| config = qwen25_500m_pretrain_config() | ||
|
|
||
| pretrain(config, forward_step, callbacks=manager) | ||
| ``` |
There was a problem hiding this comment.
Documentation clarity: code example is not self-contained.
This code block references MyCallback(), TimingCallback(), and MetricsCallback() (line 74-75) but none of these are defined or imported in this example. While MyCallback is defined in the first example, TimingCallback and MetricsCallback are not defined anywhere in this document.
For better documentation, consider either:
- Adding a comment noting these are example callback names and users should define them
- Importing or defining these callbacks in the example
- Using simpler inline examples that don't reference undefined classes
📝 Suggested improvement for clarity
```python
from megatron.bridge.training.callbacks import CallbackManager
from megatron.bridge.training.gpt_step import forward_step
from megatron.bridge.training.pretrain import pretrain
from megatron.bridge.recipes.qwen import qwen25_500m_pretrain_config
+# Assuming MyCallback is defined as shown in the Quick Start example above
+# and you have defined TimingCallback and MetricsCallback for your use case
+
manager = CallbackManager()
manager.add(MyCallback())
-manager.add([TimingCallback(), MetricsCallback()])
+manager.add([MyOtherCallback()]) # Add any other callback instances
manager.register("on_eval_end", lambda ctx: print("Evaluation complete!"))🤖 Prompt for AI Agents
In `@docs/training/callbacks.md` around lines 67 - 82, The example uses undefined
callback classes (MyCallback, TimingCallback, MetricsCallback) when creating
CallbackManager and calling manager.add/register; update the snippet to be
self-contained by either importing/defining the referenced callbacks or adding a
short inline comment above the manager.add calls stating these are
placeholder/example callback names that must be defined or imported by the user
(e.g., "Assuming MyCallback/TimingCallback/MetricsCallback are defined
elsewhere"), and ensure references to CallbackManager, manager.add, and
manager.register remain correct.
beep boop [🤖]: Hi @ananthsub 👋,
Summary by CodeRabbit