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

Fix broken Hugging Face link #1

Merged
merged 2 commits into from
Jan 31, 2024
Merged

Fix broken Hugging Face link #1

merged 2 commits into from
Jan 31, 2024

Conversation

thenameless7741
Copy link
Contributor

@thenameless7741 thenameless7741 commented Jan 30, 2024

The original link to the models on Hugging Face website was broken, so this PR just fixes it.

Edit 1: I didn't have git-lfs installed when I ran download-models, and I encountered an error while checking out the model repo. So, I added an extra note about that.

Edit 2: despite having $HF_TOKEN set and being logged into Hugging Face (verified by make setup), I couldn't successfully run download-models due to an auth error. So I fixed the Makefile to include HF's username and token when cloning the model's repository (I'm not familiar with huggingface-cli, so I'm not sure how to properly fix this). Side note: users who have set an SSH key on the HF website shouldn't run into this auth issue.

ps. Thank you for open sourcing this project!

@thenameless7741 thenameless7741 changed the title doc: fix broken HuggingFace link Fix broken Hugging Face link Jan 30, 2024
Copy link
Contributor

@ZachNagengast ZachNagengast left a comment

Choose a reason for hiding this comment

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

Awesome @thenameless7741 thanks for PR #1 🎉
This looks like a great fix, just recommended a check in case the user is in an HF org (it shows up as a second line)

Could also be reduced to just

HF_USERNAME=$(huggingface-cli whoami | head -n 1) \

What do you think?

Makefile Outdated Show resolved Hide resolved
@ZachNagengast ZachNagengast self-requested a review January 31, 2024 04:17
Copy link
Contributor

@ZachNagengast ZachNagengast left a comment

Choose a reason for hiding this comment

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

Looks great, nicely done!

@ZachNagengast ZachNagengast merged commit 0958393 into argmaxinc:main Jan 31, 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

2 participants