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

Discussion: unwrap tracker #1109

Merged
merged 1 commit into from
Feb 24, 2023
Merged

Discussion: unwrap tracker #1109

merged 1 commit into from
Feb 24, 2023

Conversation

pcuenca
Copy link
Member

@pcuenca pcuenca commented Feb 24, 2023

Just opening this PR for discussion, related to the changes in 38fd30e

Observations:

My question is, is the user expected to pass unwrap=True (and, therefore, there's a slight API change here), or is this being caused by some other issue? How is unwrap meant to work? The change in this PR makes it work for me, but I'm not sure if this is the intended way. Happy to dig into this further once the expectations have been clarified :)

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Feb 24, 2023

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

@muellerzr
Copy link
Collaborator

muellerzr commented Feb 24, 2023

Yes, it's likely unwrap should be True by default, but the goal is to entice users to use the returned Accelerate tracker API instead as it handles everything. unwrap=True and the if...else... should be used if you want to grab the internal tracker, which is an API change.

Let me stew on this though, there may be something else I can do.

Edit: since the goal is to make the user have to think about if they want the internal tracking mechanism, a default of False is still what should happen :)

@muellerzr
Copy link
Collaborator

However, your solution Is actually perfect so going to go ahead and merge it :)

@pcuenca pcuenca merged commit f054799 into main Feb 24, 2023
@pcuenca pcuenca deleted the tracker-unwrap branch February 24, 2023 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants