Conversation
…rain_end When synchronous checkpoint saving is used (`ckpt_async_save=False`), the underlying `checkpoint_io` is the base `MLFlashpointCheckpointIO` class, which does not possess the `maybe_finalize_save_checkpoint` method. This method is specifically provided by the async wrapper `MLFlashpointAsyncFinalizableCheckpointIO`. Previously, the `on_train_end` callback unconditionally called this method, leading to an `AttributeError` and causing unsuccessfull checkpointing. Change-Id: I5e9bbf698b8ac5137f7ab3a6a723ac1e7cd3c085
kkkapu
requested review from
Leahlijuan and
g-husam
and removed request for
g-husam
April 23, 2026 20:37
g-husam
reviewed
Apr 23, 2026
g-husam
reviewed
Apr 27, 2026
Change-Id: Id24bf94613dd567645fe3c2bb31f1291468e1011
Change-Id: Iaaa84c8d96c923ad3c6254e102ebe7b0422a5a73
g-husam
reviewed
Apr 30, 2026
| mock_checkpoint_io = mocker.MagicMock() | ||
|
|
||
| # Remove the async-specific method to simulate a synchronous CheckpointIO instance. | ||
| del mock_checkpoint_io.maybe_finalize_save_checkpoint |
Collaborator
There was a problem hiding this comment.
dont think this represents sync saving correctly? Should just use MLFlashpointCheckpointIO directly (not wrapped in AsyncFinalizable) right?
I would actually expect this test to fail now in its current form
Collaborator
There was a problem hiding this comment.
I see why it doesnt fail, its testing a mock impl, not the actual code, which isnt of type AsyncFinalizableCheckpointIO. Make sure to use a real MLFlashpointCHeckpointIO - this test is not actually validating our logic, its just using a mock that gets ignored
Change-Id: I5650ff22f3c0c4ce41a56eaab3990c788ee09a4a
Python Code Coverage Summary
Minimum allowed line rate is |
C++ Code Coverage Summary
Minimum allowed line rate is |
g-husam
approved these changes
Apr 30, 2026
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
When synchronous checkpoint saving is used (
ckpt_async_save=False), the underlyingcheckpoint_iois the baseMLFlashpointCheckpointIOclass, which does not possess themaybe_finalize_save_checkpointmethod. This method is specifically provided by the async wrapperMLFlashpointAsyncFinalizableCheckpointIO.Previously, the
on_train_endcallback unconditionally called this method, leading to anAttributeErrorand causing unsuccessfull checkpointing.Change-Id: I5e9bbf698b8ac5137f7ab3a6a723ac1e7cd3c085