Setting default values in training job config #104
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Thara Palanivel <[email protected]>
Signed-off-by: Thara Palanivel <[email protected]>
Signed-off-by: Thara Palanivel <[email protected]>
Signed-off-by: Thara Palanivel <[email protected]>
tharapalanivel
requested review from
anhuong,
Ssukriti and
alex-jw-brooks
as code owners
Signed-off-by: Thara Palanivel <[email protected]>
anhuong
reviewed
Apr 1, 2024
Ssukriti
reviewed
Apr 1, 2024
Ssukriti
reviewed
Apr 1, 2024
Ssukriti
requested changes
Apr 1, 2024
Signed-off-by: Thara Palanivel <[email protected]>
Signed-off-by: Thara Palanivel <[email protected]>
Signed-off-by: Thara Palanivel <[email protected]>
Ssukriti
previously approved these changes
Apr 2, 2024
There was a problem hiding this comment.
approved from my end, just a remaining question on default for peft_method^. - I want to ensure we are testing fine tuning correctly, peft_method should be None for fine tuning.
- Does peft_method need to be passed explicitly as None then for fine tuning?
- or should we keep default to None, and pass "pt"/"Lora" explictly for prompt tuning/LoRA?
I think second one makes more sense.
When @anhuong approves, we can merge
Signed-off-by: Thara Palanivel <[email protected]>
anhuong
previously approved these changes
Apr 2, 2024
Comment on lines
75
to
78
| with open(json_path, "r", encoding="utf-8") as f: | ||
| contents = json.load(f) | ||
| peft_method_parsed = contents.get("peft_method") | ||
| logging.debug("Input params parsed: %s", contents) | ||
| job_config_dict = json.load(f) | ||
| elif json_env_var: | ||
| job_config_dict = txt_to_obj(json_env_var) |
There was a problem hiding this comment.
Note to self: perhaps we should refactor this into build/utils as well since accelerate_launch and launch_training use the same method to parse the JSON
There was a problem hiding this comment.
Good point! I'll add that in the follow-up tests PR that is in the queue next
Signed-off-by: Thara Palanivel <[email protected]>
Signed-off-by: Thara Palanivel <[email protected]>
anhuong
approved these changes
Apr 2, 2024
anhuong
pushed a commit
to anhuong/fms-hf-tuning
that referenced
this pull request
Apr 3, 2024
…#104) * Allow for default params to be set Signed-off-by: Thara Palanivel <[email protected]> * Add tests Signed-off-by: Thara Palanivel <[email protected]> * Simplifying default params logic Signed-off-by: Thara Palanivel <[email protected]> * Setting use_flash_attn default Signed-off-by: Thara Palanivel <[email protected]> * Formatting Signed-off-by: Thara Palanivel <[email protected]> * Address review comments Signed-off-by: Thara Palanivel <[email protected]> * Moving tests Signed-off-by: Thara Palanivel <[email protected]> * Address review comments Signed-off-by: Thara Palanivel <[email protected]> * Address review comment Signed-off-by: Thara Palanivel <[email protected]> * Fix merge conflicts Signed-off-by: Thara Palanivel <[email protected]> --------- Signed-off-by: Thara Palanivel <[email protected]> Signed-off-by: Anh-Uong <[email protected]>
anhuong
pushed a commit
to anhuong/fms-hf-tuning
that referenced
this pull request
Apr 3, 2024
…#104) * Allow for default params to be set Signed-off-by: Thara Palanivel <[email protected]> * Add tests Signed-off-by: Thara Palanivel <[email protected]> * Simplifying default params logic Signed-off-by: Thara Palanivel <[email protected]> * Setting use_flash_attn default Signed-off-by: Thara Palanivel <[email protected]> * Formatting Signed-off-by: Thara Palanivel <[email protected]> * Address review comments Signed-off-by: Thara Palanivel <[email protected]> * Moving tests Signed-off-by: Thara Palanivel <[email protected]> * Address review comments Signed-off-by: Thara Palanivel <[email protected]> * Address review comment Signed-off-by: Thara Palanivel <[email protected]> * Fix merge conflicts Signed-off-by: Thara Palanivel <[email protected]> --------- Signed-off-by: Thara Palanivel <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the change
Setting the following defaults for training jobs, will get overwritten if user explicitly passes in a value
save_strategy=epochlogging_strategy=epochRelated issue number
How to verify the PR
Was the PR tested