[jit] Remove try/catch in constant propagation #19686
Conversation
| // we cannot actually represent the IValue as a constant node, | ||
| // so we give up replacing it | ||
| } else { | ||
| // If we cannot insert the IValue as a |
There was a problem hiding this comment.
you probably don't need the empty else branch here (but retain the comment please)
I'm not sure it should return an optional. It's only the case of constant propagation in which it throws |
In general, it is bad practice to use exceptions in cases where a failed result is expected (i.e. not exceptional). It's true that insertConstant is used in different contexts, some where failure is expected and some where it isn't. But I think that means we should take the lowest common denominator |
|
Can we have a tryInsertConstant and insertConstant maybe ? There are 90 callsites of insertConstant. Only one of them has any expectation of failure. It would be pretty annoying to have to assert success on all 89 |
|
I think you are confusing with graph::insertConstant, I see only six uses: edit: I see that CSE uses graph's insertConstant as well so you're right that it's effectively 90. Anyway, tryInsert and insert seems like a good compromise to me |
| // so we give up replacing it | ||
| } else { | ||
| // If we cannot insert the IValue as a | ||
| // constant, give up replacing it and let DCE remove it. |
There was a problem hiding this comment.
Well DCE is not guaranteed to remove this node if we fail to do this, so this comment is a bit misleading (or I am missing something).
There was a problem hiding this comment.
Yes, DCE is removing n, not the IValue, as in the original version of the comment
| // auto new_output = tryInsertConstant(graph, outputs[i]); | ||
| if (outputs[i].isNone()) { | ||
| new_output->setType(n->outputs()[i]->type()); | ||
| (*new_output)->setType(n->outputs()[i]->type()); |
There was a problem hiding this comment.
Can you not do new_output->setType... ? would think pointer works here but maybe not. Also, semi related if you don't mind adding a comment above throw c10::Error("Can't insert requires grad as to say something along the lines of // error gets caught within constant prop
|
You still didn't handle the two cases of AT_ASSERT for a Tensor or TensorList that requires grad |
|
Those are actual bugs, we shouldn't ever be trying to make a variable a constant, so I think they are still useful as is. |
Summary: Stack from [ghstack](https://github.com/ezyang/ghstack): * **pytorch#19686 [jit] Remove try/catch in constant propagation** The try-catch here gets tripped pretty often when constant prop is run which screws up `catch throw` in gdb.](https://our.intern.facebook.com/intern/diff/15170134/) Pull Request resolved: pytorch#19686 Pulled By: driazati Differential Revision: D15170134 fbshipit-source-id: 93688561126f3ab582c8358e8f2787f7fce9aa73
6 participants
Stack from ghstack:
The try-catch here gets tripped pretty often when constant prop is run which screws up
catch throwin gdb.Differential Revision: D15170134