-
Notifications
You must be signed in to change notification settings - Fork 11
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
Generic reports #377
Generic reports #377
Conversation
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
src/modelbench/run.py
Outdated
"--custom-branding", | ||
type=click.Path(file_okay=False, dir_okay=True, path_type=pathlib.Path), | ||
help="Path to custom branding. Implicitly sets --generic-report to remove MLCommons copy.", | ||
) | ||
@click.option( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make more sense here to just have one CLI option that is either 1) --custom-branding
which includes MLC branded stuff or 2) no --custom-branding
which is always generic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't users need to know the path of MLC branding in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for now the only users running reports with MLC branding will be the three of us. And we'll eventually put that branding directory in modellab
, where we'll be running the official reports from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I like where you're going with this, but agree with Daniel. The default should be a generic report. Then something like a --branding option lets us point to something like a folder that contains our flavor packet. In a future pull request, we'll move that folder-or-whatever to ModelLab, but this PR just needs to start extracting it.
Okay, updated to remove |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Sorry it took so long for me to review!
A possibly wild stab at generating generic reports. content/ now contains all of the generic/default .toml files, and values in the new content_mlc/ directory will override the defaults if
--generic-report
is not provided.Just want to confirm that this is generally how we want generic reports to work, and then I’ll add the CSS stuff.