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

suts-for-benchmark-page #22

Merged
merged 1 commit into from
Dec 15, 2023
Merged

suts-for-benchmark-page #22

merged 1 commit into from
Dec 15, 2023

Conversation

dhosterman
Copy link
Collaborator

Add styling and content for pages that show Benchmark outputs for various SUTs, using the most recently provided wireframe as reference.

@dhosterman dhosterman requested a review from a team as a code owner December 14, 2023 17:23
@dhosterman dhosterman requested a review from wpietri December 14, 2023 17:23
@dhosterman dhosterman self-assigned this Dec 14, 2023
Copy link

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

Copy link
Contributor

@wpietri wpietri left a comment

Choose a reason for hiding this comment

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

Overall, looks good to me. I think the tests are right and all of this is a step forward. My only quibble is the notion of hardcoding a use of /tmp, which I think will be hard for people to find and doesn't exist on Windows. Maybe it's time for a proper click CLI where the default output path is "./web" but it's overrideable with a CLI option?

@dhosterman
Copy link
Collaborator Author

My only quibble is the notion of hardcoding a use of /tmp, which I think will be hard for people to find and doesn't exist on Windows. Maybe it's time for a proper click CLI where the default output path is "./web" but it's overrideable with a CLI option?

Yeah, my assumption here is that everything in the main block is just a proxy for the eventual CLI invocation, so that path is a very temporary concession to not having a way to configure the output location. I agree that it's probably about time we either add a CLI or some cheap configuration via env vars.

@dhosterman dhosterman merged commit baf9849 into main Dec 15, 2023
2 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 15, 2023
@wpietri wpietri deleted the suts-for-benchmark-page branch April 2, 2024 20:45
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.

2 participants