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.

Sign up for GitHub

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

Jump to bottom

fixing the full state path in checkpoint handler+loss report calculation #51

Merged
merged 16 commits into from
Aug 1, 2023
Merged

fixing the full state path in checkpoint handler+loss report calculation #51

merged 16 commits into from
Aug 1, 2023

Conversation

HamidShojanazeri
Copy link
Contributor

@HamidShojanazeri HamidShojanazeri commented Jul 26, 2023 •
edited
Loading

This PR fixes

This was referenced Jul 26, 2023
Closed
Closed
chauhang
chauhang reviewed Jul 29, 2023
View reviewed changes
utils/train_utils.py Outdated Show resolved Hide resolved
chauhang
chauhang reviewed Jul 29, 2023
View reviewed changes
utils/train_utils.py Outdated Show resolved Hide resolved
@HamidShojanazeri HamidShojanazeri mentioned this pull request Jul 30, 2023
Closed
@HamidShojanazeri HamidShojanazeri changed the title fixing the full state path in checkpoint handler fixing the full state path in checkpoint handler+loss report calculation Jul 30, 2023
chauhang
chauhang reviewed Jul 30, 2023
View reviewed changes
utils/train_utils.py Outdated Show resolved Hide resolved
chauhang
chauhang reviewed Jul 30, 2023
View reviewed changes
utils/train_utils.py Outdated Show resolved Hide resolved
chauhang
chauhang reviewed Jul 30, 2023
View reviewed changes
utils/train_utils.py Outdated Show resolved Hide resolved
chauhang
chauhang suggested changes Jul 30, 2023
View reviewed changes
Copy link
Contributor

@chauhang chauhang left a comment

Choose a reason for hiding this comment

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

@HamidShojanazeri Thanks for the fixes. Please see comments inline.

@HamidShojanazeri HamidShojanazeri mentioned this pull request Jul 31, 2023
Closed
@DhruvaBansal00
Copy link

Seems like this is supposed to fix #65? I am going to try this out and will keep you folks updated!

@HamidShojanazeri
Copy link
Contributor Author

@DhruvaBansal00 thanks for trying out the PR, I am not able to repro on my end can you pls share the command your running? and you env usingpython -m "torch.utils.collect_env".

@DhruvaBansal00
Copy link

@HamidShojanazeri I hadn't applied your changes correctly before. Just finished running three epochs and seems like everything is running as expected. Thanks for the changes and appreciate the fast turnaround :)

@HamidShojanazeri
Copy link
Contributor Author

@HamidShojanazeri I hadn't applied your changes correctly before. Just finished running three epochs and seems like everything is running as expected. Thanks for the changes and appreciate the fast turnaround :)

Thanks very much @DhruvaBansal00 for confirmation.

chauhang
chauhang approved these changes Aug 1, 2023
View reviewed changes
Copy link
Contributor

@chauhang chauhang left a comment

Choose a reason for hiding this comment

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

Thanks @HamidShojanazeri for all the fixes and updates.

Thanks @jeonsworld your changes for PR #24 have also been rolled into this PR

@chauhang chauhang merged commit 1387b76 into main Aug 1, 2023
3 checks passed
@chauhang chauhang deleted the checkpoint_handler_path_fix branch September 1, 2023 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chauhang chauhang chauhang approved these changes

Assignees
No one assigned
Labels
cla signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@HamidShojanazeri @DhruvaBansal00 @chauhang @facebook-github-bot