-
Notifications
You must be signed in to change notification settings - Fork 27.8k
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
Refactor hyperparameter search backends #24384
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.
Thanks for your PR, this is not what we discussed on the issue which I recommended to just complete the error message to include wandb.
Using abstract classes like this is not really the way the Transformers library is designed and we don't heavily refactor everything as you did, preferring clearer code that can be more verbose but doesn;t require the reader to jump to superclasses all the time to get the behavior of a method. I also don't see how you would have benefits for IDE as you show in the PR description.
That's fine, I was very unsure which approach to take.
The point of this is that it's difficult to see all the missing bits. The current code isn't just missing wandb in the error message, it's also missing from |
Sorry for the confusion, that's not part of this PR to keep the scope focused, but if this is merged I can follow it up with another which adds constructors to each backend class which accept the precise kwargs that the backend |
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.
Thanks for bearing with me! I have additional comments but I think this is a better fit for the style of the library already.
specify name in class use methods instead of callable class attributes name constant better
…st, not module. format with black.
Opened huggingface/huggingface_hub#1526 in regards to the unrelated test failure. |
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.
Thanks for iterating! One last nit and we should be good to merge. The error in the CI is indeed unrelated.
@@ -0,0 +1,121 @@ | |||
from .integrations import ( |
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 just add a copyright here similar to all other files in the lib (potentially switching the year to 2023 if it's not)?
Thanks a lot for your contribution! |
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
Fixes #24379
The goal here is to clearly group the essential info/functionality about each backend together to make reading/changing things easier. For example if another backend integration is added it should be less likely for something to be forgotten as apparently happened with wandb.
@sgugger sorry I didn't get a full confirmation to go ahead with this, it just seemed easier to show what I meant with code rather than continue explaining in the issue. There's many other ways this could be done and I can change the approach but I hope that the general direction at least is clear from this PR.
I also think this would help move towards improving the user facing API since as mentioned in #24278 (comment) (cc @hugocool) the kwargs have no type hints and are not very easy to use. So maybe instead of:
one could write: