areusch commented on a change in pull request #7952:
URL: https://github.com/apache/tvm/pull/7952#discussion_r639966929
##########
File path: tests/python/relay/test_pass_instrument.py
##########
@@ -168,3 +169,329 @@ def run_after_pass(self, mod, info):
# Out of pass context scope, should be reset
assert passes_counter.run_before_count == 0
assert passes_counter.run_after_count == 0
+
+
+def test_enter_pass_ctx_exception(capsys):
+ @pass_instrument
+ class PI:
+ def __init__(self, id):
+ self.id = id
+
+ def enter_pass_ctx(self):
+ print(self.id + " enter ctx")
+
+ def exit_pass_ctx(self):
+ print(self.id + " exit ctx")
+
+ @pass_instrument
+ class PIBroken(PI):
+ def __init__(self, id):
+ super().__init__(id)
+
+ def enter_pass_ctx(self):
+ print(self.id + " enter ctx")
+ raise RuntimeError("Just a dummy error")
+
+ pass_ctx = tvm.transform.PassContext(instruments=[PI("%1"),
PIBroken("%2"), PI("%3")])
+ with pytest.raises(tvm.error.TVMError):
+ with pass_ctx:
+ pass
+
+ assert "%1 enter ctx\n" "%2 enter ctx\n" == capsys.readouterr().out
Review comment:
> I am not quite understanding the sentence - i don't think you can
implement __enter__ with knowledge of any following __enter___.
The tricky thing is that you're making `PassContext::EnterWithScope` into a
composite with statement--in other words, calling `EnterWithScope` calls
multiple `PassInstrument::EnterPassContext`. Each `EnterPassContext` can't know
whether or not another `EnterPassContext` is going to error, so doing things
like opening files isn't 100% safe here--a following `EnterPassContext` which
errors would leave the file open. Depending on the severity of the error, the
user could retry compilation and run into a more complex, harder-to-reproduce
error.
However, in every other error case (i.e. if errors occur in shouldRun,
beforePass, pass, afterPass), `EnterPassContext` can count on `ExitPassContext`
running. So I'd just suggest to add logic to call `ExitPassContext` while
looping through and calling `EnterPassContext`. I added a comment where I think
it should go.
i think if you do that, then that will button up this interface and it
should handle errors in the same way as you'd expect given logic in
`include/tvm/support/with.h`. You can test this by adding the `%1 exit ctx`
here.
lmk if that seems unreasonable, i could be missing something. i do think
that fixing the hole in the ExitPassContext is worth the potential complexity
of the double-error, because at least the user is working with a simpler
abstraction layer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
