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

feat: enable resume download for hf_hub_download #1249

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

lonelam
Copy link
Contributor

@lonelam lonelam commented Nov 15, 2023

enable auto resume download by default to prevent always restart on downloading large file with poor network connection.

Copy link
Contributor

@lopagela lopagela left a comment

Choose a reason for hiding this comment

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

Suggesting to make it False by default in settings.py to do not explicitly require it in the chain of settings*.yaml

private_gpt/settings/settings.py Outdated Show resolved Hide resolved
settings.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@pabloogc pabloogc left a comment

Choose a reason for hiding this comment

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

I think going for a very simple solution here is way better. Just making it always resume the download seems like a healthy default.

That way you don't have to overcomplicate with a setting that realistically nobody will need to change.

@lonelam
Copy link
Contributor Author

lonelam commented Nov 16, 2023

@pabloogc , thank you for your review and suggestions. I agree that resuming downloads by default is a prudent approach. However, to address scenarios where resuming may not be ideal, such as when the model file in the HuggingFace repository has been updated, I propose adding an optional setting in the LocalSettings class. This would allow users to choose whether to resume downloads from the last breakpoint or start afresh, thereby ensuring flexibility and robustness in handling different situations.

@pabloogc
Copy link
Collaborator

@pabloogc , thank you for your review and suggestions. I agree that resuming downloads by default is a prudent approach. However, to address scenarios where resuming may not be ideal, such as when the model file in the HuggingFace repository has been updated, I propose adding an optional setting in the LocalSettings class. This would allow users to choose whether to resume downloads from the last breakpoint or start afresh, thereby ensuring flexibility and robustness in handling different situations.

I still wouldn't expose it as a setting, instead pass a parameter to the setup script scripts/setup --resume=true/false defaulting to true should be enough for 99.9% of scenarios.

@lonelam
Copy link
Contributor Author

lonelam commented Nov 16, 2023

Well, '--resume'/'--no-resume' are added as arguments for the setup script in the new commit. The default value is true.

@pabloogc pabloogc merged commit 4197ada into zylon-ai:main Nov 16, 2023
6 checks passed
simonbermudez pushed a commit to simonbermudez/saimon that referenced this pull request Feb 24, 2024
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.

None yet

3 participants