Skip to content

Make DDP Quantization hooks backend Agnostic #138816

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.

Sign up for GitHub

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

Jump to bottom
Closed
VRSinghHabana wants to merge 1 commit into from
Closed

Make DDP Quantization hooks backend Agnostic #138816

VRSinghHabana wants to merge 1 commit into from

Conversation

@VRSinghHabana
Copy link
Contributor

@VRSinghHabana VRSinghHabana commented Oct 24, 2024 •
edited by pytorch-bot bot
Loading

Current ddp hooks quantization code use .cuda() API to move tensors and parameter on backend devices. This limits only cuda backend to work with ddp quantization hooks.
Change is to make code backend agnostic and move tensors/parameters based on tensor.device.

cc @H-Huang @awgu @kwen2501 @wanchaol @fegin @fduwjj @wz337 @wconstab @d4l3k @c-p-i-o

@pytorch-bot
Copy link

pytorch-bot bot commented Oct 24, 2024 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/138816

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 714097b with merge base d2ec289 (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@pytorch-bot pytorch-bot bot added oncall: distributed Add this issue/PR to distributed oncall triage queue release notes: distributed (ddp) release notes category labels Oct 24, 2024
@fegin fegin added ciflow/trunk Trigger trunk jobs on your pull request ciflow/periodic Trigger jobs ran periodically on master (periodic.yml) on the PR labels Oct 24, 2024
fegin
fegin reviewed Oct 24, 2024
View reviewed changes
Copy link
Contributor

@fegin fegin left a comment

Choose a reason for hiding this comment

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

LGTM, I'll wait for the CI signals.

@VRSinghHabana
Copy link
Contributor Author

VRSinghHabana commented Oct 28, 2024

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Oct 28, 2024

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Oct 28, 2024

Successfully rebased ddp_hooks onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout ddp_hooks && git pull --rebase)

@ezyang ezyang added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Oct 28, 2024
@VRSinghHabana
Copy link
Contributor Author

VRSinghHabana commented Oct 29, 2024

@fegin , CI is passed, Could you pls merge the change.

kwen2501
kwen2501 approved these changes Oct 29, 2024
View reviewed changes
Copy link
Collaborator

@kwen2501 kwen2501 left a comment

Choose a reason for hiding this comment

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

LGTM

torch/distributed/algorithms/ddp_comm_hooks/quantization_hooks.py


def _quantize_per_tensor_cuda(x, scale, zero_point):
def _quantize_per_tensor_backend(x, scale, zero_point):
Copy link
Collaborator

@kwen2501 kwen2501 Oct 29, 2024

Choose a reason for hiding this comment

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

nit: maybe just name it _quantize_per_tensor?

torch/distributed/algorithms/ddp_comm_hooks/quantization_hooks.py


def _quantize_per_channel_cuda(x, scale, zero_point):
def _quantize_per_channel_backend(x, scale, zero_point):
Copy link
Collaborator

@kwen2501 kwen2501 Oct 29, 2024

Choose a reason for hiding this comment

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

nit: maybe just name it _quantize_per_channel?

@VRSinghHabana VRSinghHabana requested a review from fegin October 29, 2024 07:35
@VRSinghHabana
Copy link
Contributor Author

VRSinghHabana commented Oct 29, 2024

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Oct 29, 2024

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

rahulsingh-intel pushed a commit to rahulsingh-intel/pytorch that referenced this pull request Nov 5, 2024
@VRSinghHabana @rahulsingh-intel
Make DDP Quantization hooks backend Agnostic (pytorch#138816)
0de9f34 
Current ddp hooks quantization code use .cuda() API to move tensors and parameter on backend devices. This limits only cuda backend to work with ddp quantization hooks.
Change is to make code backend agnostic and move tensors/parameters based on **tensor.device.**

Pull Request resolved: pytorch#138816
Approved by: https://github.com/kwen2501
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kwen2501 kwen2501 kwen2501 approved these changes

@fegin fegin Awaiting requested review from fegin

Assignees

No one assigned

Labels

ciflow/periodic Trigger jobs ran periodically on master (periodic.yml) on the PR ciflow/trunk Trigger trunk jobs on your pull request Merged oncall: distributed Add this issue/PR to distributed oncall triage queue open source release notes: distributed (ddp) release notes category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@VRSinghHabana @pytorchmergebot @fegin @kwen2501 @ezyang @pytorchbot