-
Notifications
You must be signed in to change notification settings - Fork 1k
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 Tracker logic and write guards for logging_dir #316
Conversation
The documentation is not available anymore as the PR was closed or merged. |
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 fixing! The logic for all the logging in the init of Accelerator is starting to take a lot of code, so I think we should refactor it in a separate method to keep the init cleam.
self.trackers.append(WandBTracker(project_name)) | ||
elif str(tracker).lower() == "comet_ml" and is_comet_ml_available(): | ||
self.trackers.append(CometMLTracker(project_name)) | ||
else: |
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.
Very nice!
@sgugger pinging for a rereview just to make sure the refactor seems sound to you 😄 |
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, thanks for refactoring the bit that gets all loggers outside of the Accelerator!
self.logging_dir = logging_dir | ||
self.log_with = filter_trackers(log_with, self.logging_dir) |
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.
Much cleaner!
Refactor
logging_dir
logic and make better guards for trackers that need oneWhat does this add?
This PR refactors much of the logic when it comes to how Trackers are initialized and checked inside of
Accelerate
Who is it for?
Why is it needed?
I noticed that users were finding it confusing as to why a
logging_dir
must always be passed in when using"all"
, and this was because ofTensorBoard
requiring a logging directory.On top of this, the current logic to check if a tracker can be added is a heap of spaghetti code that isn't maintainable.
What parts of the API does this impact?
User-facing:
When users forget to pass in a
logging_dir
toAccelerate.__init__
and a particular tracker needs them, an error is now raised stating:Internal structure:
Each
GeneralTracker
implementation now requires that therequires_logging_directory
be set, as this is a foundational method for checking whetherlogging_dir
should be passed. As a result this is an abstract property, not a default (as the user should know explicitly).There also now exists a
LOGGER_TYPE_TO_CLASS
dictionary to severely reduce the boilerplate code needed inAccelerator.init_trackers
when it comes to checking if a logging_dir argument is needed