Adding a version serialization type to ConvPackedParam #43086
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[ghstack-poisoned]
facebook-github-bot
added
the
oncall: jit
💊 CI failures summary and remediationsAs of commit 8e54f74 (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 139 times. |
vkuzo
changed the title
…ackedParam" Note: this is a debug copy of #40003 [ghstack-poisoned]
vkuzo
added a commit
that referenced
this pull request
Aug 21, 2020
ghstack-source-id: 4130686 Pull Request resolved: #43086 [rfc] temporary custom pickle functions for maintaining BC on ConvPackedParams Summary: This is an another approach to changing ConvPackedParams structure while maintaining BC. We add the ability to add custom pickle functions where `__set_state__` takes a `c10::IValue` and knows how to parse it, and define the custom pickle methods with the parsing logic on `ConvPackedParams`. There are various TODOs before land left in the diff, ideally we can align if this is the approach people are OK with and implement the TODOs after consensus. For additional context, #43087 contained an alternative version of this. Test Plan: For now, check that saving a model with https://gist.github.com/vkuzo/f3616c5de1b3109cb2a1f504feed69be on an old revision and loading it on this revision works. Tests would be needed if this approach sounds good. Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 4130686 Pull Request resolved: #43230
…ackedParam" Note: this is a debug copy of #40003 [ghstack-poisoned]
…ackedParam" Note: this is a debug copy of #40003 [ghstack-poisoned]
vkuzo
added a commit
that referenced
this pull request
Aug 23, 2020
ghstack-source-id: 4bf1bfd Pull Request resolved: #43086 [rfc] temporary custom pickle functions for maintaining BC on ConvPackedParams Summary: This is an another approach to changing ConvPackedParams structure while maintaining BC. We add the ability to add custom pickle functions where `__set_state__` takes a `c10::IValue` and knows how to parse it, and define the custom pickle methods with the parsing logic on `ConvPackedParams`. There are various TODOs before land left in the diff, ideally we can align if this is the approach people are OK with and implement the TODOs after consensus. For additional context, #43087 contained an alternative version of this. Test Plan: For now, check that saving a model with https://gist.github.com/vkuzo/f3616c5de1b3109cb2a1f504feed69be on an old revision and loading it on this revision works. Tests would be needed if this approach sounds good. Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 4bf1bfd Pull Request resolved: #43230
…ackedParam" Note: this is a debug copy of #40003 [ghstack-poisoned]
…ackedParam" Note: this is a debug copy of #40003 [ghstack-poisoned]
vkuzo
added a commit
that referenced
this pull request
Aug 23, 2020
ghstack-source-id: 2996bf1 Pull Request resolved: #43086 [rfc] temporary custom pickle functions for maintaining BC on ConvPackedParams Summary: This is an another approach to changing ConvPackedParams structure while maintaining BC. We add the ability to add custom pickle functions where `__set_state__` takes a `c10::IValue` and knows how to parse it, and define the custom pickle methods with the parsing logic on `ConvPackedParams`. There are various TODOs before land left in the diff, ideally we can align if this is the approach people are OK with and implement the TODOs after consensus. For additional context, #43087 contained an alternative version of this. Test Plan: For now, check that saving a model with https://gist.github.com/vkuzo/f3616c5de1b3109cb2a1f504feed69be on an old revision and loading it on this revision works. Tests would be needed if this approach sounds good. Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 2996bf1 Pull Request resolved: #43230
…ackedParam" Note: this is a debug copy of #40003 [ghstack-poisoned]
…ackedParam" Note: this is a debug copy of #40003 [ghstack-poisoned]
vkuzo
added a commit
that referenced
this pull request
Aug 24, 2020
ghstack-source-id: 77ccb81 Pull Request resolved: #43086 [rfc] temporary custom pickle functions for maintaining BC on ConvPackedParams Summary: This is an another approach to changing ConvPackedParams structure while maintaining BC. We add the ability to add custom pickle functions where `__set_state__` takes a `c10::IValue` and knows how to parse it, and define the custom pickle methods with the parsing logic on `ConvPackedParams`. There are various TODOs before land left in the diff, ideally we can align if this is the approach people are OK with and implement the TODOs after consensus. For additional context, #43087 contained an alternative version of this. Test Plan: For now, check that saving a model with https://gist.github.com/vkuzo/f3616c5de1b3109cb2a1f504feed69be on an old revision and loading it on this revision works. Tests would be needed if this approach sounds good. Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 77ccb81 Pull Request resolved: #43230
…ackedParam" Note: this is a debug copy of #40003 [ghstack-poisoned]
…ackedParam" Note: this is a debug copy of #40003 [ghstack-poisoned]
…ackedParam" Note: this is a debug copy of #40003 [ghstack-poisoned]
vkuzo
added a commit
that referenced
this pull request
Aug 24, 2020
ghstack-source-id: 63d65f2 Pull Request resolved: #43086 [rfc] temporary custom pickle functions for maintaining BC on ConvPackedParams Summary: This is an another approach to changing ConvPackedParams structure while maintaining BC. We add the ability to add custom pickle functions where `__set_state__` takes a `c10::IValue` and knows how to parse it, and define the custom pickle methods with the parsing logic on `ConvPackedParams`. There are various TODOs before land left in the diff, ideally we can align if this is the approach people are OK with and implement the TODOs after consensus. For additional context, #43087 contained an alternative version of this. Test Plan: For now, check that saving a model with https://gist.github.com/vkuzo/f3616c5de1b3109cb2a1f504feed69be on an old revision and loading it on this revision works. Tests would be needed if this approach sounds good. Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 63d65f2 Pull Request resolved: #43230
vkuzo
changed the title
dreiss
reviewed
Aug 27, 2020
Comment on lines
262
to
265
| "Unexpected length of conv_params_packed, expected ", | ||
| conv_params_packed.numel(), | ||
| " got ", | ||
| idx); |
Contributor
There was a problem hiding this comment.
I think the message here is reversed. idx is what we expected.
Contributor
Author
There was a problem hiding this comment.
you're right, thanks!
Codecov Report
@@ Coverage Diff @@
## gh/vkuzo/124/base #43086 +/- ##
==================================================
Coverage 69.33% 69.34%
==================================================
Files 378 378
Lines 46698 46698
==================================================
+ Hits 32380 32381 +1
+ Misses 14318 14317 -1
Continue to review full report at Codecov.
|
This PR changes the format of `ConvPackedParam` in a nearly backwards-compatible way: * a new format is introduced which has more flexibility and a lower on-disk size * custom pickle functions are added to `ConvPackedParams` which know how to load the old format * the custom pickle functions are **not** BC because the output type of `__getstate__` has changed. We expect this to be acceptable as no user flows are actually broken (loading a v1 model with v2 code works), which is why we whitelist the failure. For a short forward compatibility period, we keep the `__getstate__` function with the old format, while `__getstate__` knows about both old and new format. The next PR in the stack will be landed some period after this PR, and will switch `__getstate__` to the new format. Note: the bc test cases were generated with the `v2` format, to allow manual testing of the `v2` code in this PR. Test plan: ``` // adhoc testing of saving v1 and loading in v2: https://gist.github.com/vkuzo/f3616c5de1b3109cb2a1f504feed69be // test that loading models with v1 conv params format works and leads to the same numerics python test/test_quantization.py TestSerialization.test_conv2d_graph python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph // test that saving and loading models with v2 conv params format works and leads to same numerics python test/test_quantization.py TestSerialization.test_conv2d_graph_v2 python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph_v2 // test that numerics on quantized MobileNetV2 do not change: // https://gist.github.com/vkuzo/d593385a88c30e22f46eba4248070c79 ``` Note: this is a newer copy of #40003 Differential Revision: [D23347832](https://our.internmc.facebook.com/intern/diff/D23347832) [ghstack-poisoned]
vkuzo
added a commit
that referenced
this pull request
Aug 27, 2020
Summary: This is a forward compatibility follow-up to #43086. We switch the conv serialization to output the v2 format instead of the v1 format. The plan is to land this 1 - 2 weeks after the base PR. Test Plan: ``` python test/test_quantization.py TestSerialization.test_conv2d_graph_v2 python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph_v2 ``` Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D23355480](https://our.internmc.facebook.com/intern/diff/D23355480) [ghstack-poisoned]
vkuzo
added a commit
that referenced
this pull request
Aug 27, 2020
Summary: This is a forward compatibility follow-up to #43086. We switch the conv serialization to output the v2 format instead of the v1 format. The plan is to land this 1 - 2 weeks after the base PR. Test Plan: ``` python test/test_quantization.py TestSerialization.test_conv2d_graph_v2 python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph_v2 ``` Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: c5bd1ff Pull Request resolved: #43651
This PR changes the format of `ConvPackedParam` in a nearly backwards-compatible way: * a new format is introduced which has more flexibility and a lower on-disk size * custom pickle functions are added to `ConvPackedParams` which know how to load the old format * the custom pickle functions are **not** BC because the output type of `__getstate__` has changed. We expect this to be acceptable as no user flows are actually broken (loading a v1 model with v2 code works), which is why we whitelist the failure. For a short forward compatibility period, we keep the `__getstate__` function with the old format, while `__getstate__` knows about both old and new format. The next PR in the stack will be landed some period after this PR, and will switch `__getstate__` to the new format. Note: the bc test cases were generated with the `v2` format, to allow manual testing of the `v2` code in this PR. Test plan: ``` // adhoc testing of saving v1 and loading in v2: https://gist.github.com/vkuzo/f3616c5de1b3109cb2a1f504feed69be // test that loading models with v1 conv params format works and leads to the same numerics python test/test_quantization.py TestSerialization.test_conv2d_graph python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph // test that saving and loading models with v2 conv params format works and leads to same numerics python test/test_quantization.py TestSerialization.test_conv2d_graph_v2 python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph_v2 // test that numerics on quantized MobileNetV2 do not change: // https://gist.github.com/vkuzo/d593385a88c30e22f46eba4248070c79 ``` Note: this is a newer copy of #40003 Differential Revision: [D23347832](https://our.internmc.facebook.com/intern/diff/D23347832) [ghstack-poisoned]
vkuzo
added a commit
that referenced
this pull request
Aug 28, 2020
Summary: This is a forward compatibility follow-up to #43086. We switch the conv serialization to output the v2 format instead of the v1 format. The plan is to land this 1 - 2 weeks after the base PR. Test Plan: ``` python test/test_quantization.py TestSerialization.test_conv2d_graph_v2 python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph_v2 ``` Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D23355480](https://our.internmc.facebook.com/intern/diff/D23355480) [ghstack-poisoned]
vkuzo
added a commit
that referenced
this pull request
Aug 28, 2020
Summary: This is a forward compatibility follow-up to #43086. We switch the conv serialization to output the v2 format instead of the v1 format. The plan is to land this 1 - 2 weeks after the base PR. Test Plan: ``` python test/test_quantization.py TestSerialization.test_conv2d_graph_v2 python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph_v2 ``` Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 2534b3b Pull Request resolved: #43651
This PR changes the format of `ConvPackedParam` in a nearly backwards-compatible way: * a new format is introduced which has more flexibility and a lower on-disk size * custom pickle functions are added to `ConvPackedParams` which know how to load the old format * the custom pickle functions are **not** BC because the output type of `__getstate__` has changed. We expect this to be acceptable as no user flows are actually broken (loading a v1 model with v2 code works), which is why we whitelist the failure. For a short forward compatibility period, we keep the `__getstate__` function with the old format, while `__getstate__` knows about both old and new format. The next PR in the stack will be landed some period after this PR, and will switch `__getstate__` to the new format. Note: the bc test cases were generated with the `v2` format, to allow manual testing of the `v2` code in this PR. Test plan: ``` // adhoc testing of saving v1 and loading in v2: https://gist.github.com/vkuzo/f3616c5de1b3109cb2a1f504feed69be // test that loading models with v1 conv params format works and leads to the same numerics python test/test_quantization.py TestSerialization.test_conv2d_graph python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph // test that saving and loading models with v2 conv params format works and leads to same numerics python test/test_quantization.py TestSerialization.test_conv2d_graph_v2 python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph_v2 // test that numerics on quantized MobileNetV2 do not change: // https://gist.github.com/vkuzo/d593385a88c30e22f46eba4248070c79 ``` Note: this is a newer copy of #40003 Differential Revision: [D23347832](https://our.internmc.facebook.com/intern/diff/D23347832) [ghstack-poisoned]
vkuzo
added a commit
that referenced
this pull request
Aug 28, 2020
Summary: This is a forward compatibility follow-up to #43086. We switch the conv serialization to output the v2 format instead of the v1 format. The plan is to land this 1 - 2 weeks after the base PR. Test Plan: ``` python test/test_quantization.py TestSerialization.test_conv2d_graph_v2 python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph_v2 ``` Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D23355480](https://our.internmc.facebook.com/intern/diff/D23355480) [ghstack-poisoned]
vkuzo
added a commit
that referenced
this pull request
Aug 28, 2020
Summary: This is a forward compatibility follow-up to #43086. We switch the conv serialization to output the v2 format instead of the v1 format. The plan is to land this 1 - 2 weeks after the base PR. Test Plan: ``` python test/test_quantization.py TestSerialization.test_conv2d_graph_v2 python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph_v2 ``` Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 864f62e Pull Request resolved: #43651
Contributor
vkuzo
added a commit
that referenced
this pull request
Sep 8, 2020
Summary: This is a forward compatibility follow-up to #43086. We switch the conv serialization to output the v2 format instead of the v1 format. The plan is to land this 1 - 2 weeks after the base PR. Test Plan: ``` python test/test_quantization.py TestSerialization.test_conv2d_graph_v2 python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph_v2 ``` Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D23355480](https://our.internmc.facebook.com/intern/diff/D23355480) [ghstack-poisoned]
vkuzo
added a commit
that referenced
this pull request
Sep 8, 2020
Summary: This is a forward compatibility follow-up to #43086. We switch the conv serialization to output the v2 format instead of the v1 format. The plan is to land this 1 - 2 weeks after the base PR. Test Plan: ``` python test/test_quantization.py TestSerialization.test_conv2d_graph_v2 python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph_v2 ``` Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: e57f6c6 Pull Request resolved: #43651
vkuzo
added a commit
that referenced
this pull request
Sep 8, 2020
Summary: This is a forward compatibility follow-up to #43086. We switch the conv serialization to output the v2 format instead of the v1 format. The plan is to land this 1 - 2 weeks after the base PR. Test Plan: ``` python test/test_quantization.py TestSerialization.test_conv2d_graph_v2 python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph_v2 ``` Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D23355480](https://our.internmc.facebook.com/intern/diff/D23355480) [ghstack-poisoned]
vkuzo
added a commit
that referenced
this pull request
Sep 9, 2020
Summary: This is a forward compatibility follow-up to #43086. We switch the conv serialization to output the v2 format instead of the v1 format. The plan is to land this 1 - 2 weeks after the base PR. Test Plan: ``` python test/test_quantization.py TestSerialization.test_conv2d_graph_v2 python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph_v2 ``` Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D23355480](https://our.internmc.facebook.com/intern/diff/D23355480) [ghstack-poisoned]
vkuzo
added a commit
that referenced
this pull request
Sep 9, 2020
Summary: This is a forward compatibility follow-up to #43086. We switch the conv serialization to output the v2 format instead of the v1 format. The plan is to land this 1 - 2 weeks after the base PR. Test Plan: ``` python test/test_quantization.py TestSerialization.test_conv2d_graph_v2 python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph_v2 ``` Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D23355480](https://our.internmc.facebook.com/intern/diff/D23355480) [ghstack-poisoned]
vkuzo
added a commit
that referenced
this pull request
Sep 9, 2020
Summary: This is a forward compatibility follow-up to #43086. We switch the conv serialization to output the v2 format instead of the v1 format. The plan is to land this 1 - 2 weeks after the base PR. Test Plan: ``` python test/test_quantization.py TestSerialization.test_conv2d_graph_v2 python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph_v2 ``` Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 5cf3b89 Pull Request resolved: #43651
vkuzo
added a commit
that referenced
this pull request
Sep 9, 2020
Summary: This is a forward compatibility follow-up to #43086. We switch the conv serialization to output the v2 format instead of the v1 format. The plan is to land this 1 - 2 weeks after the base PR. Test Plan: ``` python test/test_quantization.py TestSerialization.test_conv2d_graph_v2 python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph_v2 ``` Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D23355480](https://our.internmc.facebook.com/intern/diff/D23355480) [ghstack-poisoned]
vkuzo
added a commit
that referenced
this pull request
Sep 9, 2020
Summary: This is a forward compatibility follow-up to #43086. We switch the conv serialization to output the v2 format instead of the v1 format. The plan is to land this 1 - 2 weeks after the base PR. Test Plan: ``` python test/test_quantization.py TestSerialization.test_conv2d_graph_v2 python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph_v2 ``` Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D23355480](https://our.internmc.facebook.com/intern/diff/D23355480) [ghstack-poisoned]
vkuzo
added a commit
that referenced
this pull request
Sep 9, 2020
Summary: This is a forward compatibility follow-up to #43086. We switch the conv serialization to output the v2 format instead of the v1 format. The plan is to land this 1 - 2 weeks after the base PR. Test Plan: ``` python test/test_quantization.py TestSerialization.test_conv2d_graph_v2 python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph_v2 ``` Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 181a12c Pull Request resolved: #43651
vkuzo
added a commit
that referenced
this pull request
Sep 10, 2020
Summary: This is a forward compatibility follow-up to #43086. We switch the conv serialization to output the v2 format instead of the v1 format. The plan is to land this 1 - 2 weeks after the base PR. Test Plan: ``` python test/test_quantization.py TestSerialization.test_conv2d_graph_v2 python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph_v2 ``` Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D23355480](https://our.internmc.facebook.com/intern/diff/D23355480) [ghstack-poisoned]
vkuzo
added a commit
that referenced
this pull request
Sep 10, 2020
Summary: This is a forward compatibility follow-up to #43086. We switch the conv serialization to output the v2 format instead of the v1 format. The plan is to land this 1 - 2 weeks after the base PR. Test Plan: ``` python test/test_quantization.py TestSerialization.test_conv2d_graph_v2 python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph_v2 ``` Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D23355480](https://our.internmc.facebook.com/intern/diff/D23355480) [ghstack-poisoned]
vkuzo
added a commit
that referenced
this pull request
Sep 10, 2020
Summary: This is a forward compatibility follow-up to #43086. We switch the conv serialization to output the v2 format instead of the v1 format. The plan is to land this 1 - 2 weeks after the base PR. Test Plan: ``` python test/test_quantization.py TestSerialization.test_conv2d_graph_v2 python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph_v2 ``` Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 297cfe0 Pull Request resolved: #43651
facebook-github-bot
pushed a commit
that referenced
this pull request
Sep 10, 2020
Summary: Pull Request resolved: #43651 This is a forward compatibility follow-up to #43086. We switch the conv serialization to output the v2 format instead of the v1 format. The plan is to land this 1 - 2 weeks after the base PR. Test Plan: ``` python test/test_quantization.py TestSerialization.test_conv2d_graph_v2 python test/test_quantization.py TestSerialization.test_conv2d_nobias_graph_v2 ``` Imported from OSS Reviewed By: z-a-f Differential Revision: D23355480 fbshipit-source-id: 4cb04ed8b90a0e3e452297a411d641a15f6e625f
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stack from ghstack:
This PR changes the format of
ConvPackedParamin a nearly backwards-compatible way:ConvPackedParamswhich know how to load the old format__getstate__has changed. We expect this to be acceptable as no user flows are actually broken (loading a v1 model with v2 code works), which is why we whitelist the failure.For a short forward compatibility period, we keep the
__getstate__function with the old format,while
__getstate__knows about both old and new format. The next PR in the stack will be landedsome period after this PR, and will switch
__getstate__to the new format.Note: the bc test cases were generated with the
v2format, to allow manual testing of thev2code in this PR.
Test plan:
Note: this is a newer copy of #40003
Differential Revision: D23347832