Skip to content

Conversation

goanpeca
Copy link
Collaborator

@goanpeca goanpeca commented Aug 29, 2025

@github-actions github-actions bot added module: engine Engine module module: base Base module module: metrics Metrics module module: handlers Core Handlers module ci CI labels Aug 29, 2025
@goanpeca goanpeca marked this pull request as ready for review August 29, 2025 19:12
@Copilot Copilot AI review requested due to automatic review settings August 29, 2025 19:12
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a fix for max_iters handling in the Engine state dictionary and adds comprehensive test coverage. The changes extend the Engine class to properly support max_iters as an alternative to max_epochs for controlling training duration.

Key changes include:

  • Enhanced Engine state dict validation to support both max_epochs and max_iters termination modes
  • Updated serialization logic to include appropriate termination parameters based on the training configuration
  • Extended test coverage for various max_iters scenarios including resuming, state loading, and edge cases

Reviewed Changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tests/ignite/engine/test_max_iters_fix.py Comprehensive test suite for max_iters functionality covering state dict operations, resuming, and validation
tests/ignite/engine/test_engine_state_dict.py Updated existing tests to account for additional state dict keys
tests/ignite/conftest.py Added defensive checks for pytest configuration options
tests/ignite/base/test_mixins_update.py Tests for updated Serializable mixin validation logic
ignite/engine/engine.py Core Engine implementation with max_iters support and improved state management
ignite/base/mixins.py Enhanced Serializable base class with grouped optional key validation
Various workflow and metric files Minor fixes and updates

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@goanpeca goanpeca force-pushed the fix/max-iters branch 4 times, most recently from 075f57c to eba390f Compare August 29, 2025 19:37
@github-actions github-actions bot added the docs label Aug 29, 2025
@goanpeca goanpeca closed this Aug 29, 2025
@goanpeca goanpeca reopened this Aug 29, 2025
Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR @goanpeca !
I made the first pass and left comments essentially to split this PR into multiple ones and explain certain changes.
I'll check in more depth the max_iters related changes a bit later whether it works as expected.
Anyway, great job!

@goanpeca goanpeca marked this pull request as draft September 4, 2025 16:10
@goanpeca goanpeca force-pushed the fix/max-iters branch 9 times, most recently from fc1fcb5 to cfd0d13 Compare September 5, 2025 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci CI docs module: base Base module module: engine Engine module module: handlers Core Handlers module module: metrics Metrics module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible issues with max_iters when loading/saving engine's state
2 participants