Remove allowconsumed enforceconsumed from op schema.#617
Remove allowconsumed enforceconsumed from op schema.#617linkerzhang merged 25 commits intoonnx:masterfrom
Conversation
|
❌ Build onnx 0.3.1599 failed (commit 8a7ef0391e by @linkerzhang) |
|
✅ Build onnx 0.3.1600 completed (commit 26926f53d5 by @linkerzhang) |
|
Here also needs to be removed: |
|
@houseroad Models in model zoo have |
…han/remove_memory_op_from_opschema
…b.com/linkerzhang/onnx into kezhan/remove_memory_op_from_opschema
|
@bddppq Thank you for the comments! Removed the piece of codes. As you said, models in model zoo are using the consumed_inputs attribute, which is being broken. |
|
❌ Build onnx 0.3.1611 failed (commit 24a976b4e1 by @linkerzhang) |
|
Let's fix the model zoo first, will do it soon :-) Tracking here: onnx/models#26 |
|
❌ Build onnx 0.3.1645 failed (commit d5a672d5a3 by @linkerzhang) |
…han/remove_memory_op_from_opschema
|
❌ Build onnx 0.3.1722 failed (commit 4cf895eab8 by @linkerzhang) |
…b.com/linkerzhang/onnx into kezhan/remove_memory_op_from_opschema
|
❌ Build onnx 0.3.1725 failed (commit b51a82736d by @linkerzhang) |
|
❌ Build onnx 0.3.1801 failed (commit 56580e86b2 by @linkerzhang) |
|
❌ Build onnx 0.3.1813 failed (commit ffbc03ae1b by @linkerzhang) |
|
✅ Build onnx 0.3.1820 completed (commit e591e2ef08 by @linkerzhang) |
…han/remove_memory_op_from_opschema
…b.com/linkerzhang/onnx into kezhan/remove_memory_op_from_opschema
|
onnx-fb-universe error: @houseroad I think the error is not related with this change, right? |
|
@onnxbot retest this please |
| // operator schema on specific domain. Update the lowest version when it's | ||
| // determined to remove too old version history. | ||
| map_[ONNX_DOMAIN] = std::make_pair(1, 6); | ||
| map_[ONNX_DOMAIN] = std::make_pair(1, 7); |
There was a problem hiding this comment.
It has been bumped from 5 to 6 for this purpose 5f69c37
| // operator schema on specific domain. Update the lowest version when it's | ||
| // determined to remove too old version history. | ||
| map_[ONNX_DOMAIN] = std::make_pair(1, 6); | ||
| map_[ONNX_DOMAIN] = std::make_pair(1, 7); |
There was a problem hiding this comment.
Wait, as we discussed, we just keep using 6 right?
There was a problem hiding this comment.
technically, it's risky to reuse 6. as in the time in-between, there could be models using the other operators with consumed_input with version 6. :) I'm changing to use version 6 though.
|
|
||
| ONNX_OPERATOR_SCHEMA(PRelu) | ||
| .AllowConsumed({{0, 0}}) | ||
| .SinceVersion(7) |
There was a problem hiding this comment.
As we discussed, this should be .SinceVersion(6), (because in my previous PR, we already bump up the opset version, no need to do it again, otherwise, I have to prepare opset_7 model zoos, which does not exist yet... why our test case does not fail on this?
houseroad
left a comment
There was a problem hiding this comment.
We should still use 6 instead of 7, since the last opset version bumping is also due to consumed_inputs.
…han/remove_memory_op_from_opschema
…b.com/linkerzhang/onnx into kezhan/remove_memory_op_from_opschema
|
✅ Build onnx 0.3.1846 completed (commit 627c8f2451 by @linkerzhang) |
|
Thank you very much @bddppq and @houseroad a lot for reviewing the PR and helping on make it happen. All comments are resolved and I'm merging it. Let me know if there're more comments please. |
* Remove allowconsumed enforceconsumed from op schema. * update the binding part. * remove consumed attributes logic. * add consumed_input attribute in old.cc to simulate the API allowconsumed * fix build break. * simulate allowconsumed and enforceconsumed with attr api. * use vesion 6 instead of 7.
* Remove allowconsumed enforceconsumed from op schema. * update the binding part. * remove consumed attributes logic. * add consumed_input attribute in old.cc to simulate the API allowconsumed * fix build break. * simulate allowconsumed and enforceconsumed with attr api. * use vesion 6 instead of 7.
This is for issue #595 .