fix(adapter/nemo): also close alt async calls queue for regular nemo checkpoints#106
Conversation
| # causing a crash if the process group is already destroyed. | ||
| caller.close = lambda: None | ||
| # Close each queue | ||
| self._mlf_async_calls_queue.close() |
There was a problem hiding this comment.
intentionally removing the if condition, should always call this, and if it doesnt exist anymore we should fail the test so we know how to clean it up otherwise
There was a problem hiding this comment.
should we move this after line 477?
There was a problem hiding this comment.
good q, I was wondering the same. I guess the only reason to do it first is if the underlying caller close needs to be invoked when callilng this close, so figured we'd close everything normally first, then address the potential duplicate call issue after. wdyt
There was a problem hiding this comment.
have you tested on a training job? If it didn't cause any issue, either order should work
There was a problem hiding this comment.
yeah ran llama8b for 10 steps, checkpointing every 3. didnt notice any issues
| # causing a crash if the process group is already destroyed. | ||
| caller.close = lambda: None | ||
|
|
||
| self._alt_async_calls_queue.close() |
There was a problem hiding this comment.
closing the alt queue as well
Python Code Coverage Summary
Minimum allowed line rate is |
C++ Code Coverage Summary
Minimum allowed line rate is |
2 participants
close()on themlf_async_calls_queue- if the function doesn't exist we should catch that in our tests (which are updated)close()on thealt_async_calls_queue, which wasn't being closed before