[qunat][graphmode][fx] Embedding/EmbeddingBag works in static quant qconfig #48062
Conversation
…config Summary: When Embedding/EmbeddingBag are configured with static quant we'll skip inserting observer for them in the graph and keep the op unchanged and print a warning. This also aligns with eager mode behavior as well. We'll enforce this behavior for other ops that only supports dynamic/weight_only quant but not static quant as well. Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
…tic quant qconfig" Summary: When Embedding/EmbeddingBag are configured with static quant we'll skip inserting observer for them in the graph and keep the op unchanged and print a warning. This also aligns with eager mode behavior as well. We'll enforce this behavior for other ops that only supports dynamic/weight_only quant but not static quant as well. We used a global variable `DEFAULT_NOT_OBSERVED_QUANTIZE_HANDLER`, this is not exposed to user right now, we can add that later if needed. Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
jerryzh168
requested review from
vkuzo,
z-a-f,
supriyar,
dskhudia and
raghuramank100
and removed request for
vkuzo and
z-a-f
…tic quant qconfig" Summary: When Embedding/EmbeddingBag are configured with static quant we'll skip inserting observer for them in the graph and keep the op unchanged and print a warning. This also aligns with eager mode behavior as well. We'll enforce this behavior for other ops that only supports dynamic/weight_only quant but not static quant as well. We used a global variable `DEFAULT_NOT_OBSERVED_QUANTIZE_HANDLER`, this is not exposed to user right now, we can add that later if needed. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D25007537](https://our.internmc.facebook.com/intern/diff/D25007537) [ghstack-poisoned]
💊 CI failures summary and remediationsAs of commit 372d06c (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 14 times. |
| DEFAULT_NOT_OBSERVED_QUANTIZE_HANDLER = set() | ||
| def mark_input_output_not_observed(): | ||
| def insert(fn): | ||
| DEFAULT_NOT_OBSERVED_QUANTIZE_HANDLER.add(fn) | ||
| return fn | ||
| return insert |
There was a problem hiding this comment.
question (not a suggestion that we do that, more asking for the sake of discussion): what are people's thoughts about defining this in terms of the implementation (current code), versus the use case (a mapping of ops to the quantization modes supported by those ops)?
There was a problem hiding this comment.
current code is trying to mark the QuantizeHandlers that only works for dynamic/weight_only quant to bypass the insert observer, so we don't really need to complete list. Is the suggestions to add a complete specification for all ops? what is the motivation for that? If this is trying for documentation purpose I think we probably need to list them explicitly, here we are talking about QuantizeHandler, which is an implementation detail of fx graph mode quantization, not talking about specific module/ops actually.
There was a problem hiding this comment.
my question is about the conceptual level of how the mapping is defined, where the choices are (a) defined by concepts internal to the implementation, versus (b) defined by concepts used by the end users (which op supports which quantization type). Currently this code is doing (a). A risk with (a) is it requires an understanding of the implementation to modify it, so it will be harder for people who don't work on FX graph mode full time to make changes correctly. If defined in terms of (b), that risk would be reduced.
In terms of implementation of (b), there are ways to do it with allowlist (specify all mappings) or blocklists (specify all the exceptions), I agree that using blocklists might be nicer.
It doesn't have to be in this PR, but something to consider for the future.
There was a problem hiding this comment.
Sure. I don't expect these cases to come up very often actually, we can do that if that becomes common in the future.
…tic quant qconfig" Summary: When Embedding/EmbeddingBag are configured with static quant we'll skip inserting observer for them in the graph and keep the op unchanged and print a warning. This also aligns with eager mode behavior as well. We'll enforce this behavior for other ops that only supports dynamic/weight_only quant but not static quant as well. We used a global variable `DEFAULT_NOT_OBSERVED_QUANTIZE_HANDLER`, this is not exposed to user right now, we can add that later if needed. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D25007537](https://our.internmc.facebook.com/intern/diff/D25007537) [ghstack-poisoned]
| assert dtypes in supported_dtypes, "qconfig dtype pair not supported:" \ | ||
| " {}, supported dtypes are: {}".format(dtypes, supported_dtypes) | ||
| if dtypes not in supported_dtypes: | ||
| warnings.warn("dtype combination: {} is not " |
There was a problem hiding this comment.
Can we also include the types currently supported in this warning message?
…tic quant qconfig" Summary: When Embedding/EmbeddingBag are configured with static quant we'll skip inserting observer for them in the graph and keep the op unchanged and print a warning. This also aligns with eager mode behavior as well. We'll enforce this behavior for other ops that only supports dynamic/weight_only quant but not static quant as well. We used a global variable `DEFAULT_NOT_OBSERVED_QUANTIZE_HANDLER`, this is not exposed to user right now, we can add that later if needed. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D25007537](https://our.internmc.facebook.com/intern/diff/D25007537) [ghstack-poisoned]
…tic quant qconfig" Summary: When Embedding/EmbeddingBag are configured with static quant we'll skip inserting observer for them in the graph and keep the op unchanged and print a warning. This also aligns with eager mode behavior as well. We'll enforce this behavior for other ops that only supports dynamic/weight_only quant but not static quant as well. We used a global variable `DEFAULT_NOT_OBSERVED_QUANTIZE_HANDLER`, this is not exposed to user right now, we can add that later if needed. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D25007537](https://our.internmc.facebook.com/intern/diff/D25007537) [ghstack-poisoned]
|
This pull request has been merged in d7e8384. |
Stack from ghstack:
Summary:
When Embedding/EmbeddingBag are configured with static quant we'll skip inserting observer for
them in the graph and keep the op unchanged and print a warning.
This also aligns with eager mode behavior as well.
We'll enforce this behavior for other ops that only supports dynamic/weight_only quant but not static quant as well.
We used a global variable
DEFAULT_NOT_OBSERVED_QUANTIZE_HANDLER, this is not exposed to user right now,we can add that later if needed.
Test Plan:
Reviewers:
Subscribers:
Tasks:
Tags:
Differential Revision: D25007537