-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat: Implement the API design and set up pyproject.toml for building wheel #11
Conversation
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 will need to test out the various UI's once (no integration tests yet!) to be fully sure about some of the changes, but looks good otherwise.
Signed-off-by: Angel Luu <angel.luu@us.ibm.com>
Signed-off-by: Angel Luu <angel.luu@us.ibm.com>
Signed-off-by: Angel Luu <angel.luu@us.ibm.com>
Signed-off-by: Angel Luu <angel.luu@us.ibm.com>
Signed-off-by: Angel Luu <angel.luu@us.ibm.com>
@@ -45,7 +48,9 @@ def check_lookup(self, seqlen): | |||
"batch_size": self.ta.per_device_train_batch_size, | |||
"seq_len": seqlen, | |||
"gpu_model": self.ia.gpuModel, | |||
"method": self.fm.technique, | |||
"method": "fsdp" |
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.
@ChanderG I wasn't sure about this. I saw that the data.csv
file you gave me didn't have any "full" in the method
column so I assumed maybe we are looking for "fsdp". Should I leave it as self.fm.technique.value
which is full
, or should this code be ok?
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.
We should remove the if condition and pass in technique as-is.
I only recently changed the frontend UI to display "full" instead of "fsdp". We can fix this mismatch either in the data.csv file, or in the web ui frontend. Most likely in the source data files, since "fsdp" is misleading when both are fsdp.
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 was testing the UI with the new changes and ran into a problem. This is in the "parse" function interface which you don't need for the SDK code as it stands (the user is expected to put together the config components directly in Python), but is needed in the other interfaces (UI/cli) where the input is parsed out of JSON.
Here is a minimal reproducer:
from enum import Enum
from transformers import HfArgumentParser
from dataclasses import dataclass, field
class TuningTechnique(Enum):
LORA = "lora"
FULL = "full"
@dataclass
class FMArguments:
technique: TuningTechnique = field(
default=TuningTechnique.FULL,
metadata={"help": ("Fine-tuning technique being used")},
)
config = {"technique": "lora"}
arg_parser = HfArgumentParser([FMArguments])
res = arg_parser.parse_dict(config)
print(res)
We expect the result to be a parsed FMArguments object, with the field technique an enum, but instead we get the technique field set to a string.
I think this is a limitation of HF Argparser (https://github.com/huggingface/transformers/blob/main/src/transformers/hf_argparser.py) in that it may not be possible to parse out from dict to enum like we would want.
Signed-off-by: Angel Luu <angel.luu@us.ibm.com>
Signed-off-by: Angel Luu <angel.luu@us.ibm.com>
@ChanderG Ahh good catch, I see the problem. I spent some time today trying to figure out how to get enum to work with JSON/dict parsing, but couldn't. So I switched it back to using str. Let me know if this is good to merge! |
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.
LGTM.
Run:
to build a wheel
Then install it:
And run the sdk example:
This PR: