linkedin / linkedin/Liger-Kernel

[RFC] More robust revert functions for convergence tests

Open
#636 0 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
6.6k
Forks
603
Avg merge
1d 20h
Merged PRs (30d)
47

Description

### 🐛 Describe the bug

Many discussions show that the current revert functions have several limitations, including:

- incomplete revert: https://github.com/linkedin/Liger-Kernel/pull/627#issuecomment-2757281103 #542
- not automatically updating old reference: #385
- comparing wrong references: https://github.com/linkedin/Liger-Kernel/pull/627#issuecomment-2756914629

Simply reloading the modules would generate new class/function objects different from the original ones, i.e. `revert(patch(object)) != object`, which can easily lead to bugs when doing comparisons (`isinstance()`), repatching, reusing configs and so on.

One way to address the issue is having a patch manager for testing to keep track of replaced objects, so we can easily revert back to the original objects after monkey patch. Or is there a way to achieve it with pytest?

cc @tyler-romero @austin362667

### Reproduce

_No response_

### Versions

none

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the current revert functions used by the convergence tests, then review the linked discussions in PR #627 and issues #542 and #385. Define a testing approach that preserves original object identity, updates old references, and compares the correct references; done means these listed limitations are covered by reproducible tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
testing-qa
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.