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

Remove redundant code in checkpoint conversion script #1547

Merged
merged 2 commits into from
Jul 3, 2024

Conversation

rasbt
Copy link
Collaborator

@rasbt rasbt commented Jul 2, 2024

Remove some unused code in the HF conversion code to estimate file sizes

@rasbt rasbt requested review from Andrei-Aksionov and removed request for lantiga and awaelchli July 2, 2024 17:18
@Andrei-Aksionov
Copy link
Collaborator

There are other 2 comments in the #1534 that I've left.

@rasbt
Copy link
Collaborator Author

rasbt commented Jul 3, 2024

Thanks @Andrei-Aksionov, I saw one and added it, and the other one was the following?

Maybe we could just retrieve the number of layers from the config file, monitor when the conversion script started working on another layer and use this as a signal to update the progress bar?

I like that idea and actually also attempted it, but in practice it would be a bit trickier and require more restructuring. Given that there are so many other important things to add to and improve in LitGPT, I thought it wouldn't be worth going down this rabbit hole now. The file-size based approach seems to get the job done well for now, what do you think?

@Andrei-Aksionov
Copy link
Collaborator

I like how you implemented it, so no need to do any changes.

@rasbt
Copy link
Collaborator Author

rasbt commented Jul 3, 2024

Great, thanks 😊

@rasbt rasbt merged commit 43145e8 into main Jul 3, 2024
9 checks passed
@rasbt rasbt deleted the remove-redundant-code branch July 3, 2024 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants