-
Notifications
You must be signed in to change notification settings - Fork 168
return UserError when selected wrong optim arg instead of SystemError #4684
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
Conversation
Test Results for assets-test0 tests 0 ✅ 0s ⏱️ Results for commit 148b087. ♻️ This comment has been updated with latest results. |
@microsoft-github-policy-service agree company="Microsoft" |
|
Can you also please add a sanity run in description? The changes look good to me. |
c08a928 to
332af4e
Compare
Not able to do it since, the only options are adamw_torch, adafactor(which are valid), whereas in UI it also has adamw_hf which will give the error, but when triggered from UI it takes released component |
|
@KathyayaniT |
| def validate_optimiser_name(args: Namespace) -> None: | ||
| """Validate optimiser name allowing only 'adamw_torch' and 'adafactor'.""" | ||
| optim = str(args.optim).lower() | ||
| if optim not in ("adamw_torch", "adafactor"): |
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 this be a static list? just in case tomorrow if we add another optimiser updating the list would be sufficient? accordingly update the error message to construct a string from this list instead of hardcoding in the log?
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.
yeah, make senses, will change that into list
updated |
|
This pull request has been marked as stale because it has been inactive for 14 days. |
|
This pull request has been automatically closed due to inactivity. |
No description provided.