I saw the comment in notebook 09_learner about how “contextlib.context_manager has a surprising “feature” which doesn’t let us raise an exception before the yield” and how _CbCtxInner should fix this, so I tried to test it by impementing a simple CB that skips a particular batch, for example:
class SkipCB(Callback):
order = Callback.order - 1
def __init__(self, n): fc.store_attr()
def before_batch(self, learner): if learner.iter == self.n: raise CancelBatchException()
From my testing this CB doesn´t work, because the CancelBatchException() is never handled.
From what I understand after some debugging, what happens is that the CancelBatchException() is raised in the __enter__ method from self.cb_ctx('batch'), but it is first catched only by the __exit__ method from self.cb_ctx('epoch') and later by the __exit__ method from self.cb_ctx('fit'). The problem is the line return exc_type==chk_exc in these methods always returns False, since CancelBatchException() != CancelEpochException() != CancelFitException(), therefore the exception is never handled.
Thanks for digging into this @pereira.lucas! Perhaps we should revert to using decorators instead of a context manager? Might that help avoid this complexity?
I’m not completely familiar with decorators and context managers to say with certainty that decorators are a better (easier to read and/or more efficient) route, but this is a solution I found with decorators after your suggestion, Jeremy:
We would still need to implement the [...]_loop functions to be decorated, but it does seem at least more consise then the solution with context managers.
I don´t remember seeing a CB implementation in the course22p2 using decorators, when you say “revert” are you referring to something in the course I missed or to a previous implementation in fastai?
Jeremy, are you accepting PRs in the course repo or is it just for the instructors?
I ask because I never contributed to an opensource project so it would be a great exercise for me, but as a professor I can totally understand if the repo is just for the instructors.
But if you are accepting PRs, do you think this change back to decorators would be an ok PR or do you prefer to just checkout earlier versions of the Learner implementation?
I am indeed accepting PRs, thank you for the offer! Although in this case, I’d rather do some experiments myself to see what I can come up with, rather than merging a PR.