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

Coffee uses newhelm #95

Merged
merged 10 commits into from
Feb 15, 2024
Merged

Coffee uses newhelm #95

merged 10 commits into from
Feb 15, 2024

Conversation

wpietri
Copy link
Contributor

@wpietri wpietri commented Feb 15, 2024

This does basically the same thing as before, but using NewHelm instead of Helm. Note that NewHelm is much more fragile, so you may have to try any given run a number of times to get it to work, but it does work for me on occasion. Note also that the --web-only option doesn't work for the moment, as we'll have to discuss whether coffee should just save scores or whether it's better to pull things out of the newhelm run data.

@wpietri wpietri requested a review from dhosterman February 15, 2024 01:11
Copy link

github-actions bot commented Feb 15, 2024

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

@wpietri wpietri merged commit 898d037 into main Feb 15, 2024
2 checks passed
@wpietri wpietri deleted the coffee-uses-newhelm branch February 15, 2024 15:21
@github-actions github-actions bot locked and limited conversation to collaborators Feb 15, 2024
Copy link
Member

@mkly mkly left a comment

Choose a reason for hiding this comment

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

Awesome to see this land! A few super tiny things since this is already merged anyway.



@dataclasses.dataclass
class NewSutDescription:
Copy link
Member

@mkly mkly Feb 15, 2024

Choose a reason for hiding this comment

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

Did we maybe mean for this to be NewhelmSutDescription?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but I should change it to just SutDescription now that the old SutDescription has been removed.

#
# Recreates the test data for the things that use newhelm results loaded from disk.
#
# You shouldn't normally need to run this. If you do, perhaps because of a strucutral change
Copy link
Member

Choose a reason for hiding this comment

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

So minor but: strucutural structural

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.

tests/make_data.py Show resolved Hide resolved
tests/test_benchmark.py Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants