Skip to content

fix(train): show resumed position in epoch progress bar#850

Open
imargulis wants to merge 2 commits into
vllm-project:mainfrom
imargulis:fix/resume-progress-bar
Open

fix(train): show resumed position in epoch progress bar#850
imargulis wants to merge 2 commits into
vllm-project:mainfrom
imargulis:fix/resume-progress-bar

Conversation

@imargulis

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #603, which added mid-epoch checkpoint save and resume.

The fast-skip resume path removes already completed batches from the training
loader. Because tqdm inferred the reduced iterable length, the progress bar starts
from zero even though training had resumed mid-epoch.

Initialize the rank-zero progress bar with the full epoch total and the resumed
local step so it displays the correct percentage and remaining work.

Also adds a regression test covering the progress-bar arguments for a fully
skipped resumed epoch.

Testing

  • tests/unit/train/test_mid_epoch_resume.py

Signed-off-by: Igor Margulis <igor.margulis@intel.com>
@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: c109c337-0964-4311-8d2e-dfc0501121ce

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Changes

Mid-epoch progress reporting

Layer / File(s) Summary
Initialize progress at the resume step
src/speculators/train/trainer.py, tests/unit/train/test_mid_epoch_resume.py
Trainer.train_epoch initializes tqdm with the epoch total and skipped-step offset; a unit test verifies the progress bar arguments during mid-epoch resume.

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: showing the resumed position in the epoch progress bar.
Description check ✅ Passed The description matches the implemented fix and regression test for mid-epoch resume progress-bar behavior.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/speculators/train/trainer.py`:
- Around line 432-436: In src/speculators/train/trainer.py lines 432-436, keep
the requested resume offset separate from the number of batches actually skipped
by the fast-skip path, and pass the actual skipped count to tqdm’s initial value
so fallback replay starts at zero. In tests/unit/train/test_mid_epoch_resume.py
lines 408-448, add coverage using a sampler without fast-skip that verifies
progress initialization remains zero during replay.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4a35a412-bab7-4a33-966d-7d4f8c3a62c6

📥 Commits

Reviewing files that changed from the base of the PR and between a9bc119 and 5f624d5.

📒 Files selected for processing (2)
  • src/speculators/train/trainer.py
  • tests/unit/train/test_mid_epoch_resume.py

Comment thread src/speculators/train/trainer.py Outdated
Signed-off-by: Igor Margulis <igor.margulis@intel.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@mergify

mergify Bot commented Jul 24, 2026

Copy link
Copy Markdown

Merge Protections

🔴 1 of 1 protections blocking · waiting on 👀 reviews

Protection Waiting on
🔴 Require approval from approved reviewers list 👀 reviews

🔴 Require approval from approved reviewers list

Waiting for any of

  • approved-reviews-by = dsikka
  • approved-reviews-by = fynnsu
  • approved-reviews-by = orestis-z
  • approved-reviews-by = rahul-tuli
  • approved-reviews-by = shanjiaz
This rule is failing.

All pull requests must have at least one approving review from a member of the approved reviewers list before merging.

  • any of:
    • approved-reviews-by = dsikka
    • approved-reviews-by = fynnsu
    • approved-reviews-by = orestis-z
    • approved-reviews-by = rahul-tuli
    • approved-reviews-by = shanjiaz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant