linkedin / linkedin/Liger-Kernel

`revert_liger_kernel_to_xxx` can't revert LigerCrossEntropyLoss for transformers>=4.46.1

Open
#542 7 comments 2 reactions 0 assignees View on GitHub
bug
Dominant language
Python
Stars
6.6k
Forks
603
Avg merge
1d 20h
Merged PRs (30d)
47

Description

### 🐛 Describe the bug

#369 found that CrossEntropyLoss wasn't applied in post-grad-acc-fix versions of transformers. Despite the fact that #375 fixed the issue, it didn't consider the revert functions for convergence test.

Currently, the convergence test, `test_mini_models_with_logits`, is comparing two models which both are using LigerCrossEntropyLoss except the first test case. In other words, **the test results might be false positive** in the second and later test cases

The implementation of current revert functions is reloading module by calling `importlib.reload(module_name)`. We can fix the issue by carefully checking the transformers version and adding all patched modules for reloads. We should also enhance our monkey_patch unit test by adding another revert and compare, ensuring the correctness of convergence test results.

### Reproduce

Add a print statement in LigerCrossEntropyLossFunction and run
```
python3 -m pytest test/convergence/test_mini_models_with_logits.py -v -rP
```

### Versions

none

Contributor guide

Open the contributing guide

Research direction

Start by running test/convergence/test_mini_models_with_logits.py with -v -rP and inspect the revert functions that reload modules via importlib.reload. Review the monkey-patch unit test and LigerCrossEntropyLossFunction; done means the convergence cases compare correctly after each revert, including transformers versions >=4.46.1.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
machine-learning, testing
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.