Skip to content

Add terminate_iteration to skip batch handlers - #3826

Open
nightcityblade wants to merge 7 commits into
pytorch:masterfrom
nightcityblade:fix/issue-996
Open

nightcityblade wants to merge 7 commits into
pytorch:masterfrom
nightcityblade:fix/issue-996

Conversation

@nightcityblade

Copy link
Copy Markdown
Contributor

Fixes #996

Description:

Add Engine.terminate_iteration() so a process function can ignore the current batch without firing ITERATION_COMPLETED handlers that consume state.output. Apply the signal consistently to both the generator and legacy execution paths, and cover both modes with a regression test.

Tests:

  • python -m pytest tests/ignite/engine/test_engine.py -q (185 passed, 10 skipped)
  • ruff check ignite/engine/engine.py tests/ignite/engine/test_engine.py

Check list:

  • New tests are added (if a new feature is added)
  • New doc strings: description and/or example code are in RST format
  • Documentation is updated (if required)

Copilot AI 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.

Pull request overview

Adds a new Engine control signal to allow a process function to skip ITERATION_COMPLETED handlers for the current batch, preventing downstream handlers from consuming state.output when a batch should be ignored (addressing #996).

Changes:

  • Add Engine.terminate_iteration() and a new should_terminate_single_iteration flag.
  • Apply the skip logic to both generator (_run_once_on_dataset_as_gen) and legacy (_run_once_on_dataset_legacy) execution paths.
  • Add a regression test that runs in both modes via the existing interrupt_resume_enabled parameterization.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
ignite/engine/engine.py Introduces terminate_iteration() and conditionally skips Events.ITERATION_COMPLETED in both run paths.
tests/ignite/engine/test_engine.py Adds a regression test verifying skipped batches do not trigger ITERATION_COMPLETED handlers in both execution modes.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread ignite/engine/engine.py Outdated
Comment thread tests/ignite/engine/test_engine.py Outdated
@nightcityblade

Copy link
Copy Markdown
Contributor Author

Follow-up validation for 122cee3:

  • python -m pytest tests/ignite/engine/test_engine.py -q — 185 passed, 10 skipped
  • ruff check ignite/engine/engine.py tests/ignite/engine/test_engine.py — passed
  • git diff --check — passed

@aaishwarymishra

Copy link
Copy Markdown
Collaborator

Also terminate_epoch has extra parameter I notiched skip_epoch_completed, I think terminate_iteration should be consistent with that

@nightcityblade

Copy link
Copy Markdown
Contributor Author

Addressed in 54f5a5e. terminate_iteration now accepts skip_iteration_completed, defaulting to False consistently with terminate_epoch, and both execution paths honor it. The focused parametrized tests pass in generator and legacy modes (4 passed), and Ruff check/format pass.

Comment thread ignite/engine/engine.py
Comment thread ignite/engine/engine.py
@aaishwarymishra

Copy link
Copy Markdown
Collaborator

@vfdev-5 LGTM

@nightcityblade

Copy link
Copy Markdown
Contributor Author

CI note: the only failing check is linkcheck, caused by the pre-existing Polyaxon URL redirect in generated/ignite.handlers.polyaxon_logger.rst (https://polyaxon.com/docs/experimentation/tracking/client/ redirects to /docs/references/python-library/tracking/client/). The PR does not touch that documentation; all code-style, CPU, doctest, MPS, TPU, Horovod, and pyrefly jobs pass.

@vfdev-5

vfdev-5 commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

@nightcityblade thanks for your effort working on the issue. I remember it was not a simple issue as we can not simply skip iterations especially with metrics attached to engines.
Before merging this PR we should check if this work with 1) a trainer engine and a running metric attached and ensure that computed running metric is correct 2) an evaluator engine with metrics we should ensure that computed metrics are correct

@nightcityblade

Copy link
Copy Markdown
Contributor Author

Addressed in 2466d55. The terminate_iteration regression in tests/ignite/engine/test_engine.py now verifies both requested cases: skipped batches are excluded from a trainer's RunningAverage and from an evaluator's epoch Average, with the expected values asserted in both engine execution modes. Validation: 185 passed, 10 skipped for the full engine test file; Ruff check and format check passed on the changed test.

Comment thread tests/ignite/engine/test_engine.py Outdated
@aaishwarymishra

Copy link
Copy Markdown
Collaborator

sorry, I missed the updates :)

@nightcityblade

Copy link
Copy Markdown
Contributor Author

Updated the test in bba1810 to use the default RunningAverage decay while retaining the required identity output transform. The expected value is now 1.04 (only completed iterations 1 and 3 contribute with alpha=0.98). Focused result: 2 passed.

Comment thread tests/ignite/engine/test_engine.py
Co-authored-by: Aaishwarya Mishra <aaishwarymishra@gmail.com>
trainer = Engine(process)
completed_iterations = []
trainer.add_event_handler(Events.ITERATION_COMPLETED, lambda e: completed_iterations.append(e.state.iteration))
RunningAverage(output_transform=lambda output: output).attach(trainer, "running_average")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@vfdev-5 dont you think output_transform=lambda output: output should be default here like in Average so we dont have to explicitly pass it when declaring running average?

This branch has not been deployed

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

Labels

module: engine Engine module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Are there any ways to filter out or ignore a batch in engine?

4 participants