-
Notifications
You must be signed in to change notification settings - Fork 615
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve iterator checkpointing #5371
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>
Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>
Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>
Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>
szkarpinski
commented
Mar 13, 2024
restored_external_context.iterator_data.data, | ||
mock_external_context.iterator_data.data, | ||
mock_external_context.iterator_data.size), 0); | ||
daliDestroyExternalContextCheckpoint(&mock_external_context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding
daliDestroyExternalContextCheckpoint(&mock_external_context);
fixes a leak which was there before, introduced with #5356
stiepan
reviewed
Mar 15, 2024
stiepan
reviewed
Mar 15, 2024
stiepan
reviewed
Mar 15, 2024
stiepan
reviewed
Mar 15, 2024
stiepan
approved these changes
Mar 15, 2024
Co-authored-by: Kamil Tokarski <kamiltokarski04@gmail.com>
Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>
stiepan
reviewed
Mar 18, 2024
banasraf
approved these changes
Mar 18, 2024
Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>
!build |
CI MESSAGE: [13612650]: BUILD STARTED |
CI MESSAGE: [13612650]: BUILD PASSED |
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Category:
New feature (non-breaking change which adds functionality)
Refactoring (Redesign of existing code that doesn't affect functionality)
Description:
In this PR I explicitly save iterator data to the checkpoint and then use it to restore. Previously, I attempted to calculate iterator state based on pipeline state, which turned out to be a bad idea.
Iterator checkpointing now works as follows:
it.checkpoints()
pipe._iterator_data
in each of its pipelines.pipe.checkpoint()
for each of its pipelines (pipe._iterator_data
is now saved in a checkpoint as well)pipe._iterator_data
and restores its countersThis approach enables the following new features, which were problematic before:
FILL
+pad_last_batch=False
, which was not supported.reset()
in the checkpoint. Before, the fact that user has calledit.reset()
was not reflected in a checkpoint in any way, resulting in ambiguous restore.Additional information:
Affected modules and functionalities:
ExternalContext
, Base iterator, C APIKey points relevant for the review:
Tests:
test_reset
to check ifit.reset()
is now checkpointed correctly)Checklist
Documentation
DALI team only
Requirements
REQ IDs: N/A
JIRA TASK: DALI-3870