Skip to content
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

Tracker rewrite and lazy process checker #1079

Merged
merged 11 commits into from
Feb 22, 2023
Merged

Tracker rewrite and lazy process checker #1079

merged 11 commits into from
Feb 22, 2023

Conversation

muellerzr
Copy link
Collaborator

@muellerzr muellerzr commented Feb 15, 2023

This PR rewrites the tracker classes to be a regular class with init checks rather than an abstract class, letting them be flexible enough but also verifying the information that must be there is included.

This PR also makes the process execution for trackers happen at the tracker level, rather than at the Accelerator level, so that trackers can be utilized on their own. To do so, a new on_main_process decorator was added in the tracking.py specifically which checks if the tracker class has a value for on_main_process to True or False. Depending on this the logging function (and/or init) will then execute. This decorator is executed lazily, so that the PartialState will have been created not at import time.

@muellerzr muellerzr added the enhancement New feature or request label Feb 15, 2023
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Feb 15, 2023

The documentation is not available anymore as the PR was closed or merged.

@muellerzr
Copy link
Collaborator Author

Post approval I'll ping the folks at W&B to ensure they agree and like the proposed API

Copy link
Collaborator

@sgugger sgugger left a 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!

Moving the on_main_process decorator from the Accelerator to the trackers is not the right move IMO as:

  • it will break existing tracker classes not in Accelerate
  • it makes it harder for the user to write their own trackers.

src/accelerate/tracking.py Outdated Show resolved Hide resolved
@muellerzr
Copy link
Collaborator Author

As discussed offline:

I put back in @on_main_process on the Accelerator object to maintain its API, however the new API can still exist so that a user can grab the tracker from the Accelerator object and have it work OOTB without the if accelerator.is_main_process block

Comment on lines 1933 to 1939
if len(getattr(self, "trackers", [])) > 0:
for tracker in self.trackers:
if tracker.name == name:
return tracker.tracker
raise ValueError(f"{name} is not an available tracker stored inside the `Accelerator`.")
# Handle tracker only made on main process
return GeneralTracker(_blank=True)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed so that doing get_tracker doesn't return None if accelerator.init_trackers was used and the user wants to call get_tracker and not have to account for is_main_process when using the returned tracker. GeneralTracker now uses a pass so that the methods are silently called and noop

Copy link
Contributor

@pacman100 pacman100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the refactoring of the trackers! Could you please put an example code highlighting the importance of changes in this PR for clarity?

src/accelerate/tracking.py Show resolved Hide resolved
@muellerzr
Copy link
Collaborator Author

@pacman100 sure! Overall for a "general use" there isn't much impact.

But for users that want to use the trackers outside the accelerator object they can now do:

  accelerator = Accelerator(log_with="wandb")
  tracker = accelerator.get_tracker("wandb")
- with accelerator.on_main_process():
-     tracker.log(...)
+ tracker.log(...)

muellerzr and others added 5 commits February 21, 2023 08:38
Co-authored-by: Sourab Mangrulkar <13534540+pacman100@users.noreply.github.com>
@muellerzr muellerzr merged commit 38fd30e into main Feb 22, 2023
@muellerzr muellerzr deleted the tracker-rewrite branch February 22, 2023 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants