Skip to content

Conversation

@ivarflakstad
Copy link
Member

@ivarflakstad ivarflakstad commented Mar 5, 2025

This PR adds the concept of Expectations, which can be used to simplify handling different expected results depending on the environment the test is running in.

result = fn_being_tested()

expectations = Expectations({
    (None, None): "1",  // Default
    ("cuda, 8): "2"),
    ("cuda, 7): "3"),
    ("rocm, None): "4"),
)
expected = expectations.get_expectation()  # Automatically gets the correct expected result from detected environment.
assert result == expected

Addresses #36561

@ivarflakstad ivarflakstad marked this pull request as ready for review March 5, 2025 22:00
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@ydshieh
Copy link
Collaborator

ydshieh commented Mar 7, 2025

My first look has the impression that this is a bit too much. I have to think it a bit more and see what we can reduce but still get the improvements we want to have.

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree a bit with @ydshieh (who is the owner 👀 ) a bit too bloated.
Happy to have a simple and explicit function to help with these issue + an updated test to show proper usage

@ivarflakstad
Copy link
Member Author

Agree a bit with @ydshieh (who is the owner 👀 ) a bit too bloated. Happy to have a simple and explicit function to help with these issue + an updated test to show proper usage

Let me see if I can simplify it a bit. I'll fix a test with it as proper usage as well 👍

@ivarflakstad
Copy link
Member Author

@ArthurZucker @ydshieh
I've simplified quite a bit. Added usage of it as well here ☺️

@ydshieh
Copy link
Collaborator

ydshieh commented Mar 13, 2025

Hi @ivarflakstad I am wondering what's your thoughts about:

  • Remove Expectation
  • Make Expectations a subclass of dict
    • no need to have _inner and _default
    • key being (type, major), value being result
    • move _get_device_properties out to a place (as functioin) that could be reused
    • A new method named get_closed (get_best or whatever) with one argument named key
      • merge all the logic that needs to get the closet expectation inside its body and return it
    • to use it, just expection = my_expectation(_get_device_properties())

Overall, my suggestion is just to make it less abstract: just a simple class inherited a familar class (dict) with a new method get_best .

WDYK?

@ydshieh ydshieh mentioned this pull request Mar 14, 2025
Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a few nits 🙏

@ivarflakstad ivarflakstad requested a review from ydshieh March 18, 2025 13:27
Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a final nits and we are good to go! Thank you

* Matching `major` (compute capability major version) gives 2 points.
* Default expectation (if present) gives 1 points.
"""
(type, major) = key
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can think of another name other than type, it would be nice, as type is a built-in function.

@ivarflakstad ivarflakstad merged commit 706703b into main Mar 18, 2025
24 checks passed
@ivarflakstad ivarflakstad deleted the test-expectations branch March 18, 2025 22:39
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.

5 participants