Skip to content
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

[onert] Load tensor and optimizer data from the checkpoint file #13918

Merged
merged 3 commits into from
Sep 9, 2024

Conversation

jyoungyun
Copy link
Contributor

This commit loads tensor and optimizer data from the checkpoint file.

ONE-DCO-1.0-Signed-off-by: Jiyoung Yun [email protected]

Related issue: #13670
Draft: #13561

@jyoungyun jyoungyun added the PR/ready for review It is ready to review. Please review it. label Sep 3, 2024
@jyoungyun jyoungyun requested a review from a team September 3, 2024 09:30
@hseok-oh

This comment was marked as resolved.

Comment on lines 68 to 69
assert(i <= _offset.size());
return DataBufferPair{_offset[i], _size[i]};
Copy link
Contributor

Choose a reason for hiding this comment

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

Unlike std::map::operator[], std::vector::operator[] never inserts a new element into the container.

Suggested change
assert(i <= _offset.size());
return DataBufferPair{_offset[i], _size[i]};
assert(i < _offset.size());
return DataBufferPair{_offset[i], _size[i]};

// Reset EOF bit
_file.clear();

auto vindex = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

(optional)
How about using an explicit type like the above vindex?

Suggested change
auto vindex = 0;
uint32_t vindex = 0;

void updateAdamOptimizer(const std::unique_ptr<onert::exec::Execution> &exec)
{
// Adam optimizer has two optimizer variables. (mean, variance)
[[maybe_unused]] constexpr auto ADAM_VARIABLE_COUNT = 2;
Copy link
Contributor

@ragmani ragmani Sep 4, 2024

Choose a reason for hiding this comment

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

(Optional)
How about using an explicit type? This variable is used for comparing unsigned integer.

By the way, is this initializing variable as constexpr is meaningful? this variable seems to be used in runtime, not compile time.

zetwhite
zetwhite previously approved these changes Sep 6, 2024
Copy link
Contributor

@zetwhite zetwhite left a comment

Choose a reason for hiding this comment

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

I carefully read this code.
It looks good to me 👍

This commit loads tensor and optimizer data from the checkpoint file.

ONE-DCO-1.0-Signed-off-by: Jiyoung Yun <[email protected]>
ONE-DCO-1.0-Signed-off-by: Jiyoung Yun <[email protected]>
ONE-DCO-1.0-Signed-off-by: Jiyoung Yun <[email protected]>
@jyoungyun
Copy link
Contributor Author

Sorry for late update. PTAL @ragmani @zetwhite :)

Copy link
Contributor

@ragmani ragmani left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@zetwhite zetwhite left a comment

Choose a reason for hiding this comment

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

LGTM

@hseok-oh hseok-oh merged commit d1b25ee into Samsung:master Sep 9, 2024
9 checks passed
@jyoungyun jyoungyun deleted the 240903/load_checkpoint_data branch September 9, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR/ready for review It is ready to review. Please review it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants