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

[PyTorch] Add quantized packing weights support for fc & conv #3730

Closed
wants to merge 1 commit into from

Conversation

@zrphercule
Copy link
Member

zrphercule commented Nov 4, 2019

This pr added quantized conv, quantized conv_relu and quantized linear supported with packed weights & bias.
Also, conv & conv_relu has groupwise supported.
Unit test:

unit test
quantized_conv2d_test.py
quantized_conv2d_relu_test.py
quantized_linear_test.py

@zrphercule zrphercule force-pushed the zrphercule:unpack_fc branch from a4ab3db to d4264ac Nov 5, 2019
@zrphercule zrphercule changed the title [PyTorch][WIP Dont review or merge] Add quantized packing weights support for fc & conv [PyTorch]Add quantized packing weights support for fc & conv Nov 20, 2019
@zrphercule zrphercule requested review from jackm321, yinghai and qizzzh Nov 20, 2019
@zrphercule

This comment has been minimized.

Copy link
Member Author

zrphercule commented Nov 20, 2019

Also, the commit tree is a little mess now
will squash them into one at last.

Copy link

facebook-github-bot left a comment

@zrphercule has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link

facebook-github-bot left a comment

@zrphercule has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

yinghai left a comment

Overall it looks good. I think we really need to start enabling internal tests. All these tensor operations make me worry about memory issue.

torch_glow/src/PyTorchCommon.cpp Show resolved Hide resolved
torch_glow/src/PyTorchCommon.cpp Outdated Show resolved Hide resolved
torch_glow/src/PyTorchCommon.cpp Outdated Show resolved Hide resolved
torch_glow/src/PyTorchModelLoader.cpp Outdated Show resolved Hide resolved
@zrphercule

This comment has been minimized.

Copy link
Member Author

zrphercule commented Nov 21, 2019

Overall it looks good. I think we really need to start enabling internal tests. All these tensor operations make me worry about memory issue.

Agree, I maybe able to set up some e2e test when running resnet. Also we would need some unit test, I can do that later.

torch_glow/src/PyTorchCommon.cpp Show resolved Hide resolved
torch_glow/src/PyTorchCommon.cpp Show resolved Hide resolved
torch_glow/src/PyTorchCommon.cpp Outdated Show resolved Hide resolved
torch_glow/src/PyTorchCommon.cpp Outdated Show resolved Hide resolved
torch_glow/src/PyTorchCommon.cpp Outdated Show resolved Hide resolved
torch_glow/src/PyTorchModelLoader.cpp Show resolved Hide resolved
torch_glow/src/PyTorchModelLoader.cpp Outdated Show resolved Hide resolved
torch_glow/src/PyTorchModelLoader.cpp Outdated Show resolved Hide resolved
torch_glow/src/PyTorchModelLoader.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

jackm321 left a comment

LGTM!
There's are some things to fix in follow ups from noted in the comments but let's land this

torch_glow/src/PyTorchCommon.cpp Outdated Show resolved Hide resolved
.circleci/build.sh Outdated Show resolved Hide resolved
torch_glow/src/PyTorchModelLoader.cpp Outdated Show resolved Hide resolved
torch_glow/src/PyTorchModelLoader.cpp Show resolved Hide resolved
torch_glow/src/PyTorchModelLoader.cpp Show resolved Hide resolved
torch_glow/src/PyTorchModelLoader.cpp Outdated Show resolved Hide resolved
torch_glow/src/PyTorchModelLoader.cpp Show resolved Hide resolved
torch_glow/src/PyTorchCommon.cpp Show resolved Hide resolved
torch_glow/src/PyTorchModelLoader.cpp Outdated Show resolved Hide resolved
torch_glow/src/PyTorchModelLoader.cpp Outdated Show resolved Hide resolved
torch_glow/src/PyTorchModelLoader.cpp Outdated Show resolved Hide resolved
RETURN_ERR_IF_NOT(packedWeightConstant,
"quantized::linear packed weights must be constant");
const glow::Tensor &packedWeightTensor =
packedWeightConstant->getPayload().getHandle<bool>().clone();

This comment has been minimized.

Copy link
@jackm321

jackm321 Dec 3, 2019

Contributor

Did you try to remove it though?

torch_glow/src/PyTorchModelLoader.cpp Outdated Show resolved Hide resolved
torch_glow/src/PyTorchModelLoader.cpp Outdated Show resolved Hide resolved
@jackm321

This comment has been minimized.

Copy link
Contributor

jackm321 commented Dec 3, 2019

Please update the description to match the glow pr template and can you add a summary of next steps and things to clean up in there as well please (things like deleting the copied pt fbgemm code, enabling rowwise linear, etc)

@yinghai yinghai changed the title [PyTorch]Add quantized packing weights support for fc & conv [PyTorch] Add quantized packing weights support for fc & conv Dec 6, 2019
@zrphercule zrphercule force-pushed the zrphercule:unpack_fc branch from b569c0b to 7cdd23f Dec 6, 2019
@yinghai
yinghai approved these changes Dec 6, 2019
Copy link
Contributor

yinghai left a comment

Once CI passes this is good to go.

// PyTorch Tensor extracted type is kByte
// indicate it is the address of stored weights of quantized
// linear or conv.
if (ptTensor.scalar_type() == at::kByte) {

This comment has been minimized.

Copy link
@yinghai

yinghai Dec 6, 2019

Contributor

You can check its meta data to make sure it's a specific hacked tensor by doing
https://bddppq.github.io/codebrowser/pytorch/pytorch/aten/src/ATen/cpp_custom_type_hack.h.html#20

But it's fine for now.

This comment has been minimized.

Copy link
@zrphercule

zrphercule Dec 6, 2019

Author Member

yes, the meta type that custom_hack check is a type inside fbgemm's header, which require us to call fbgemm.h again. I think this should work for now, eventually I should thinking of having a registered metatype indicating this hacked type, or look its following node to make sure it is weight of conv/linear.

Copy link

facebook-github-bot left a comment

@zrphercule has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

jackm321 left a comment

LGTM!

@@ -138,12 +141,22 @@ class GlowIValue {
/// \returns Payload a vector of GlowIValues or error if the tag is not Tuple.
Expected<const std::vector<GlowIValue> *> toTuple() const;

/// \returns Payload a PyTorch Tensor* or error if the tag is not Tensor.

This comment has been minimized.

Copy link
@jackm321

jackm321 Dec 6, 2019

Contributor

nit: not a PyTorch tensor

/// a wrapper function of loadQuantizedConv and loadQuantizedConvRelu.
/// Returns error on failure.
Expected<NodeValue>
getQuantizedConvTransposeNode(const torch::jit::Node *ptNode,

This comment has been minimized.

Copy link
@jackm321

jackm321 Dec 6, 2019

Contributor

can we name this some thing like loadQuantizedConvImpl so it's clear what its doing

bool isGroupwiseQuantized = ptWeightTensor.is_quantized() &&
ptWeightTensor.qscheme() == at::kPerChannelAffine;
if (isGroupwiseQuantized) {
isGroupwiseQuantized &= (groups > 1);

This comment has been minimized.

Copy link
@jackm321

jackm321 Dec 6, 2019

Contributor

isn't this the same without the if?

This comment has been minimized.

Copy link
@zrphercule

zrphercule Dec 6, 2019

Author Member

you are right, this is some history stuff.

weightTensor.transpose(&weightTensorTransposed, NCHW2NHWC);
glow::Constant *weightConstant = F_.getParent()->createConstant(
"quantized_conv2d_weights", std::move(weightTensorTransposed));
weightConstant->ensureIsOwned();

This comment has been minimized.

Copy link
@jackm321

jackm321 Dec 6, 2019

Contributor

I'm curious if these are still needed now that pt is doing the unpacking?

This comment has been minimized.

Copy link
@zrphercule

zrphercule Dec 6, 2019

Author Member

You are right, now it is fine.

Copy link

facebook-github-bot left a comment

@zrphercule has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@zrphercule zrphercule force-pushed the zrphercule:unpack_fc branch from 4078a93 to 4545f8f Dec 6, 2019
Copy link

facebook-github-bot left a comment

@zrphercule has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link

facebook-github-bot left a comment

@zrphercule has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@zrphercule zrphercule force-pushed the zrphercule:unpack_fc branch from 209f775 to 6898e07 Dec 6, 2019
Copy link

facebook-github-bot left a comment

@zrphercule has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@zrphercule zrphercule deleted the zrphercule:unpack_fc branch Dec 8, 2019
@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Dec 10, 2019

@zrphercule merged this pull request in 93ed839.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.