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
[BUG_FIX] Fixes #6608: CHECK(data != nullptr) causes type checking to fail #6610
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One logical bug, but otherwise looks great 💯
src/relay/op/tensor/transform.cc
Outdated
@@ -1979,7 +1985,11 @@ bool StridedSliceRel(const Array<Type>& types, int num_inputs, const Attrs& attr | |||
const StridedSliceAttrs* param = attrs.as<StridedSliceAttrs>(); | |||
CHECK(param != nullptr); | |||
const auto* data = types[0].as<TensorTypeNode>(); | |||
CHECK(data != nullptr); | |||
|
|||
if (data != nullptr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (data == nullptr)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoopsies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed now.
Thanks @electriclilies @vegaluisjose @mbrookhart ! |
Some type rels check that argument types are not null. In certain cases, this can cause the type inferencer to exit prematurely, and the model cannot be run.
Instead of checking that argument types are not null, type rels should just return false, and allow the type checker to determine if the type of arguments cannot be inferred.
I believe this PR fixes all of the type rels with this issue, however I could have missed some. If you find another, please let me know.
See issue #6608 for more details.
cc @jwfromm @vegaluisjose @areusch @mbrookhart