Closed austin362667 closed 1 month ago
Hi Byron, I’m considering what tests we can add to prevent similar mistakes, such as testing against itself, from occurring in the future. I’d appreciate your feedback on this. Thanks in advance!
Thanks! Can you also update to support this new convergence test as well? https://github.com/linkedin/Liger-Kernel/blob/main/test/convergence/test_mini_models_multimodal.py
@tyler-romero Sure, done!
LGTM. I have opened a PR in your fork to relax the tol. https://github.com/austin362667/Liger-Kernel/pull/1. Please consolidate the changes
Summary
Fixes https://github.com/linkedin/Liger-Kernel/issues/176
There are several ways to restore a monkey-patched library in Python, including using context managers, decorators, pytest fixtures, or reloading the entire module.
This PR focuses on reverting monkey-patched modules when
with_liger
is disabled in convergence tests.These changes simplify the process of resetting the affected patched library and help prevent unintended side effects. And it's easier than manually reassigning functions anyway.
Follow-up
If this PR resolves the https://github.com/linkedin/Liger-Kernel/issues/176, it might introduce other value mismatch problems. We may need to adjust the convergence tolerance accordingly. For instance,
Testing Done
make test
to ensure correctnessmake checkstyle
to ensure code stylemake test-convergence
to ensure convergence