[Relay] Unifier hotfix #2437
[Relay] Unifier hotfix #2437
Conversation
…ch (not sure why this fails)
… is added, not during unification. Resolve before running a relation
…ouble-counting bug in free var pass
…for passes, fix bug in all vars pass
src/relay/pass/gradient.cc
Outdated
| * If the annotation is defined, return it. | ||
| * Otherwise, return a type hole. | ||
| */ | ||
| Type FromAnnotation(const Type& annotation) { |
There was a problem hiding this comment.
I am not sure this is the best way to handle this issue, since it is essentially duplicating what type inference does. Having undefined types inside other types is rather disconcerting to me and I think we shouldn't have them, but I am not sure how to dispel them in general. Could we make it so that a non-annotated type is made an incomplete type from the start? Would be curious as to your thoughts, @jroesch
There was a problem hiding this comment.
I think this is bug in Marisa's code, if he wants the system to do type inference for him it needs to insert incomplete types. If he wants the code to be polymorphic he needs to use polymorphism. It is invalid to have variables with p->type_annotation to null afaik.
There was a problem hiding this comment.
I think the issue is he is calling Push everywhere without providing a type, and expecting type inference, for example see line 134:
ll->Push(Add(args[i]->get<ADTensor>().reverse, rev[i]));
The push method will create a new variable with a null type annotation. I think if you change the call sites to provide an incomplete type
There was a problem hiding this comment.
I will investigate let lists, but I think the nulls are coming from function nodes and var nodes directly: A function node whose return type annotation is omitted has a null type for the ret_type field by default and a var node whose type annotation is omitted has a null type for the annotation field by default (I believe).
|
@jroesch please take another look and approve again |
|
To avoid the weird issue (I discussed offline with @jroesch) of introducing incomplete types other than in type inference, I've rewritten it so the type annotation is omitted altogether if the annotations are omitted. This should be reasonable unless it turns out there is not enough information later. |
|
Great that looks good to me. |
|
Thanks, to @jroesch @slyubomirsky , this is now merged |
This restores the changes originally in #2189, which led to an error when merged because of automatic differentiation. It turned out there was an error in the gradient code that was propagating null type annotations.