Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug where stacked decorators don't update #39

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wrongu
Copy link

@wrongu wrongu commented Feb 22, 2024

Description of the bug: The partial_credit and leaderboard decorators are class-based and use a class-based decorator pattern with @functools.wraps around an inner decorator method, along with a local callback function that is passed to the tests (i.e. set_score and set_leaderboard_value). The bug is that set_score and set_leaderboard_value update attributes of the wrong function when they are stacked. This means that only the outermost decorator with a callback function works. Example of the bug:

@partial_credit(1)
@leaderboard('a')
def test_a(set_score=None, set_leaderboard_value=None):
    set_score(1)  # works
    set_leaderboard_value(1) # fails silently, updating the __leaderboard_value__ of test_a but not of the outermost wrapper.

or, with the decorators in the other order:

@leaderboard('a')
@partial_credit(1)
def test_a(set_score=None, set_leaderboard_value=None):
    set_score(1)  # fails silently
    set_leaderboard_value(1) # works

Existing workaround: One can in principle avoid these issues by having separate partial_credit and leaderboard tests. However, this is not future-proof for adding new decorators. In fact, I discovered this bug because I added my own custom decorator for timeout, and I found that @timeout followed by @partial_credit failed while the reverse order behaved properly.

Fix: This PR fixes the issue by essentially emulating the functools.wraps behavior after the function is called (using update_wrapper), which ensures that the new values set by set_score and set_leaderboard_value are copied to the outer wrappers and thus visible to the test runner. I opted to use a context manager for this because it makes it easy to drop in a 1-line fix inside the decorators.

@wrongu wrongu requested a review from a team as a code owner February 22, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant