-
Notifications
You must be signed in to change notification settings - Fork 181
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
feat: get trainer from model_name in weight #1744
Conversation
🤖 SeineSailor Here is a concise summary of the pull request changes: Summary: This pull request introduces changes to the
Impact: These changes are internal and do not alter the external interface or behavior of the code. They address issues in pull request #1728 and issue #414, where the trainer name is extracted from the model name. Observations:
Suggestions for improvement:
|
@sunya-ch can you rebase? thanks |
@@ -1 +1 @@ | |||
{"platform": {"All_Weights": {"Bias_Weight": 220.9079278650894, "Categorical_Variables": {}, "Numerical_Variables": {"bpf_cpu_time_ms": {"scale": 5911.969193263386, "mean": 0, "variance": 0, "weight": 29.028228361462897}}}}} | |||
{"model_name": "SGDRegressorTrainer_0", "platform": {"All_Weights": {"Bias_Weight": 220.9079278650894, "Categorical_Variables": {}, "Numerical_Variables": {"bpf_cpu_time_ms": {"scale": 5911.969193263386, "mean": 0, "variance": 0, "weight": 29.028228361462897}}}}} |
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.
can we have the trainer_name instead?
I am fine with model_name but I think we shouldn't require parsing model_name to get trainer name.
func (w ComponentModelWeights) Trainer() string { | ||
if w.ModelName == "" { | ||
return "" | ||
} | ||
modelNameSplits := strings.Split(w.ModelName, "_") | ||
splitTrainer := strings.Join(modelNameSplits[0:len(modelNameSplits)-1], "_") | ||
if isSupportedTrainer(splitTrainer) { | ||
return splitTrainer | ||
} | ||
return "" | ||
} |
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.
IMHO, models should have the trainer-name metadata embedded in the json itself so that this isn't needed.
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.
model_name has more complete information since it also embed the node_type and linked to model db.
I think we can add trainer_name additionally if needed (need update on model-server side first).
Signed-off-by: Sunyanan Choochotkaew <sunyanan.choochotkaew1@ibm.com>
Related to the comment in #1728 (comment) and sustainable-computing-io/kepler-model-server#414, for now, we can extract the trainer name from the model name.
Signed-off-by: Sunyanan Choochotkaew sunyanan.choochotkaew1@ibm.com