-
Notifications
You must be signed in to change notification settings - Fork 838
katib: Update LLM HP tuning guide to clarify tunable fields and fix resource section #4067
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
base: master
Are you sure you want to change the base?
Conversation
…es config, remove unsupported params Signed-off-by: SanthoshToorpu <[email protected]>
Hi @SanthoshToorpu. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@helenxie-bit please look into the changes |
Signed-off-by: SanthoshToorpu <[email protected]>
…v1 docs instead of referring in the llm-hp file Signed-off-by: SanthoshToorpu <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
/ok-to-test
Signed-off-by: SanthoshToorpu <[email protected]>
Signed-off-by: SanthoshToorpu <[email protected]>
@andreyvelich as per @helenxie-bit instructions and after reading the thread i fixed 2/3 issues except for the params you told to remove irrelevant params. I downloaded the docker image rn but can you be more specific on what to be removed and what not? |
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.
Thank you for this contribution! It helps a lot to make the user guide clearer. Here are a few suggestions:
@@ -165,6 +109,7 @@ In addition to Hugging Face, you can integrate with S3-compatible object storage | |||
from kubeflow.storage_initializer.s3 import S3DatasetParams | |||
``` | |||
|
|||
|
|||
#### S3DatasetParams |
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.
I suggest we also move this part to the Training Operator doc and cross-reference it from this doc: https://github.com/kubeflow/website/blob/master/content/en/docs/components/trainer/legacy-v1/user-guides/fine-tuning.md
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.
The S3 data params part? I guess that has rather less relevance to training operator. However if you are referencing to the example I believe that we should have a lil snippet that gives a high level overview as most people are lazy enough to browse nested links....
I'll commit with the three params removed. But please give me a heads up wrt the above suggestion
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.
Yes, S3DatasetParams
is another way to define an external dataset, just like HuggingfaceDatasetParams
. So I think it makes sense to move its explanation into the Training Operator documentation as well.
I think we need to remove unused parameters in this part: https://www.kubeflow.org/docs/components/katib/user-guides/llm-hp-optimization/#key-parameters-for-llm-hyperparameter-tuning. Can you remove parameters objective, base_image, and parameters? Since they will not be used when optimizing hyperparameters for LLMs. |
Ref issue: kubeflow/katib#2522 |
Okay so only those threee params? |
Yes, all other parameters may be used when optimizing hyperparameters for LLMs. |
…gacy trainer docs Signed-off-by: SanthoshToorpu <[email protected]>
@helenxie-bit :Great! Maybe here it's better to use a different title since we include S3DatasetParams in this part too: how about Dataset and Model Parameter Classesin legacy trainer docs |
That sounds good to me. |
…eter classes Signed-off-by: SanthoshToorpu <[email protected]>
Please review |
Thanks for the contribution! LGTM! Please have a review when you have time @andreyvelich @mahdikhashan |
/assign |
Signed-off-by: SanthoshToorpu <[email protected]>
…es config, remove unsupported params
Checklist:
Description of your changes:



As per andrey's recommendation I added the changes mentioned in the following screenshots attached. A minimal work.
Issue
Closes: #2522
Labels
/area katib
/area website