[FEATURE] Saving HF and WandB tokens locally for improved UX - #27
Conversation
- Introduced a new Profile component that allows users to manage their Hugging Face and Weights and Biases tokens. - Implemented functionality to update and clear tokens, with local storage integration and user feedback via toast notifications. - Enhanced user experience by displaying user email and providing a logout option.
…ngReviewPage - Implemented functionality to store and retrieve Hugging Face token from local storage. - Added validation to ensure Hugging Face token is provided when exporting to HF Hub. - Updated the export configuration type to include hf_token for better integration.
…ain route - Updated the POST request handler to check for the presence of the Hugging Face token when exporting to HF Hub. - Enhanced error handling to return a 400 status with an appropriate error message if the token is missing.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@supreme-gg-gg can you please go through the above Other than that, I have tested it, but please clone this PR and test it. I believe this is ready to be merged. |
supreme-gg-gg
left a comment
There was a problem hiding this comment.
@inclinedadarsh the overall flow works well I have tested locally, I believe local storage is the simplest and most efficient as well. however, seems like there is a mismatch of the frontend types you updated and the backend schema, regarding where hf token should be injected. Pls fix the the backend routes, the type, and the UI where HF token is entered, this is good to merge afterwards
|
Regarding your things to consider:
|
- Updated the evaluation form to include a field for Hugging Face token input, required for Hugging Face models. - Implemented validation to ensure the token is provided before running evaluations. - Adjusted the API request to use the user-provided Hugging Face token instead of a default value. - Enhanced error handling to notify users when the token is missing.
- Updated the inference API to require a Hugging Face token for Hugging Face models, returning a 400 error if not provided. - Modified JobDetailPage to load the Hugging Face token from local storage and validate its presence before making inference requests. - Added a user input field for the Hugging Face token in the JobDetailPage, ensuring it is included in the API request when applicable.
- Updated the batch inference API to require a Hugging Face token for Hugging Face models, returning a 400 error if not provided. - Modified the BatchInferenceForm component to load the Hugging Face token from local storage and validate its presence before making requests. - Added a user input field for the Hugging Face token in the BatchInferenceForm, ensuring it is included in the API request when applicable. - Implemented error handling to notify users when the token is missing during inference.
now all datasets operation need hf token which user can enter
and also move it at the top in the config page
|
Done with these changes. That was a nice design catch @supreme-gg-gg! Fixed all those in the latest commit. It will be great if you can spend some time testing it and reading the code for a bit, there's a chance I might have missed something because there are just too many atoms/states on the frontend. Also, there seems to be some conflicts. I'm not sure how bad they are, I'll have a look at them and should be able to merge. |
|
Checked those conflicts, doesn't seem to be hard. I'll try to resolve them soon. If you can review the rest before that then that'll be fine, otherwise I'll push a commit by tomorrow resolving them. |
supreme-gg-gg
left a comment
There was a problem hiding this comment.
This looks good, make sure to update .env.example (push this) and .env (locally or on Vercel) to remove the token. Regarding merge conflict, did you forget to pull from main / rebase / merge before working on this because I don't think I pushed anything new to main after my last feature
|
@supreme-gg-gg there seems to be an api route That route still uses the So I believe we can just remove the I'll go ahead and remove it, and make necessary changes. Let me know if I'm making a mistake and I'll revert that. |
- Updated InferenceRequest, BatchInferenceRequest, and EvaluationRequest interfaces to make hf_token a mandatory field. - Adjusted JobDetailPage and BatchInferenceForm components to directly use hfToken without conditional checks for provider.
Oh yes I did most probably. Will have to go these conflicts now 🫠 |
|
@supreme-gg-gg the conflict has been resolved. Can you please check if everything's working properly, especially the part which has conflicts. Btw, you don't need to run it locally, it's deployed it here: https://frontend-mnrvv1knx-inclinedadarshs-projects.vercel.app/login?_vercel_share=peaI5ckNFbozqZBbHEwfuCNWjS3pcnu6 If you feel it's good to go, please go ahead and merge it. |
|
@inclinedadarsh are we enforcing required token for both unsloth and hugging face? It seems like the logic is different a different places (e.g. job vs inference). I think let's just standardise everything to always require token since that's how the backend works for now |
|
@inclinedadarsh the new commit seems to have resolved my suggestions, shall we merge this and I'll test it once more? |
Ahh no, I'm so sorry. As soon as I started testing it, the final training page started behaving very weirdly. Even though the Right now I'm triaging that issue. Can you test it yourself? You can do it here: https://frontend-68146grz8-inclinedadarshs-projects.vercel.app/dashboard?_vercel_share=TwAUtJ4T2lryWQp3f14SsYAoa6SYEnAS |
|
@supreme-gg-gg the issue is fixed now. I'm going ahead to merge it. |
Description
Fixes #14
This PR enables users to save Hugging Face token and WandB API Key locally.
Implemented changes
Things to consider