[hannk] Ensure Model transforms are valid#6317
[hannk] Ensure Model transforms are valid#6317steven-johnson wants to merge 11 commits intomasterfrom
Conversation
steven-johnson
commented
Oct 13, 2021
steven-johnson
commented
- Add a Model validity checker to ensure that all the input tensors to a given op are outputs of some previous op (or a constant)
- Revamp OpGroup::add() and remove() to properly handle nested groups (they were previously producing Models that didn't pass the new validity checker)
Minor hygiene: add explicit override annotations and enable the compiler warnings. (I was about to tweak some of the virtual functions and this has been bothering me for a while.)
- Add a Model validity checker to ensure that all the input tensors to a given op are outputs of some previous op (or a constant) - Revamp OpGroup::add() and remove() to properly handle nested groups (they were previously producing Models that didn't pass the new validity checker)
| os << "\n"; | ||
| } | ||
|
|
||
| bool Op::consumes_output_of(const Op *op) const { |
There was a problem hiding this comment.
At some point, I had a free function helper index_of_output (and index_of_input). It looks like those were probably unused and removed, but I still think they should be free functions.
There was a problem hiding this comment.
Is this a request to change to a free function?
There was a problem hiding this comment.
Yes I think it should be a free function, and return an index (or -1 if not found), we'll surely need that again soon anyways.
|
|
||
| // Inserts `to_add` in front of the first op that consumes at least one of its outputs, and returns nullptr. | ||
| // If no such op is found, does nothing and returns the original OpPtr. | ||
| [[nodiscard]] virtual OpPtr add(OpPtr to_add) { |
There was a problem hiding this comment.
Does this need to be in Op (and not OpGroup)? I think it's strange to attempt to add/remove ops from things that might not have any notion of nested ops.
There was a problem hiding this comment.
It's there strictly to avoid having to use something like cast_op in the implementation; having an empty virtual base case simplifies the implementation.
|
Gentle review ping |
|
PTAL |
Minor optimization to avoid identical overrides for OpGroup in multiple places (when all we want to process are the leaf ops)
|
Gentle review ping |
|
Obsoleted by #6379 |
2 participants
