-
Notifications
You must be signed in to change notification settings - Fork 440
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
fixed error message for GatedRepoError #1832
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchtune/1832
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 550134a with merge base 4107cc4 (): This comment was automatically generated by Dr. CI and updates every 15 minutes. |
Hi @DawiAlotaibi! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1832 +/- ##
==========================================
+ Coverage 67.30% 67.34% +0.04%
==========================================
Files 304 305 +1
Lines 16000 16015 +15
==========================================
+ Hits 10768 10785 +17
+ Misses 5232 5230 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this fix @DawiAlotaibi! Two things:
- Would you mind updating the appropriate test for this change?
- Can you sign the CLA?
Added the test cases please let me know if they are appropriate. For the CLA I am waiting on confirmation to sign it and it might take some time, will let you know once I have signed it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Thanks for such a quick fix - will merge when CI is green
Context
What is the purpose of this PR? Is it to
Please link to any issues this PR addresses.
#1831
Changelog
What are the changes made in this PR?
Checked if
args.hf_token
is passed or not. and printed a message accordingly.Test plan
None
pre-commit install
)pytest tests
pytest tests -m integration_test
UX
If your function changed a public API, please add a dummy example of what the user experience will look like when calling it.
Here is a docstring example
and a tutorial example