Skip to content

At end of epoch on an IterableDataset ensure engine.state.output rema…#3373

Merged
vfdev-5 merged 4 commits intopytorch:masterfrom
mtauraso:issue/3372-engine-state-iterable-dataset
Apr 5, 2025
Merged

At end of epoch on an IterableDataset ensure engine.state.output rema…#3373
vfdev-5 merged 4 commits intopytorch:masterfrom
mtauraso:issue/3372-engine-state-iterable-dataset

Conversation

@mtauraso
Copy link
Contributor

@mtauraso mtauraso commented Apr 5, 2025

…ins.

Fixes #3372

Description: Implements the fix described in the bugreport

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)

@github-actions github-actions bot added the module: engine Engine module label Apr 5, 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 for the PR @mtauraso , I have another suggestion here to approximatively keep the previous behavior.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Apr 5, 2025

@mtauraso can you please run code formatting script: bash ./tests/run_code_style.sh install && bash ./tests/run_code_style.sh fmt to fix:

+ flake8 ignite tests examples --config setup.cfg
ignite/engine/engine.py:1084:88: W291 trailing whitespace
ignite/engine/engine.py:1271:88: W291 trailing whitespace
ignite/engine/engine.py:1274:1: W293 blank line contains whitespace

@mtauraso
Copy link
Contributor Author

mtauraso commented Apr 5, 2025

Ran the formatter and committed. My apologies for the churn, I see now this is all documented in the first time contributors guide which I didn't read earlier.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Apr 5, 2025

Thanks for the updates and no worries about the contributing guidelines. By the way, I almost forgot about that we also need to add a test for the feature we adding. Can you please create a test here checking that state.output is not None for the iterable input data, below this line:

assert evaluator.state.iteration == size

Maybe something like this:

def test_iterator_state_output(self):
    ...

you can take a look for inspiration this test: test_faq_inf_iterator_with_epoch_length

@mtauraso mtauraso force-pushed the issue/3372-engine-state-iterable-dataset branch from a84bc24 to 9666ef3 Compare April 5, 2025 19:40
Copy link
Contributor Author

@mtauraso mtauraso left a comment

Choose a reason for hiding this comment

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

I found another bug while testing this 🙃

If there's an obvious fix I'd like to know, and would be happy for someone else to move the diff forward, but I probably won't have time to look in detail for a couple days.

If I find a fix I'll add it to the branch and ping again.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Apr 5, 2025

@mtauraso thanks for adding the tests. Let me reproduce the bug and make a separate ticket for it. This PR looks good to me and solves the initial problem. Let's land it and maybe work on the other bug in separate PR.

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.

LGTM, thanks @mtauraso !

@vfdev-5 vfdev-5 merged commit 8d850e1 into pytorch:master Apr 5, 2025
16 of 17 checks passed
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.

Engine object does not retain state.output when using an IterableDataset

2 participants