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

New front end #1947

Merged
merged 27 commits into from
Nov 11, 2023
Merged

New front end #1947

merged 27 commits into from
Nov 11, 2023

Conversation

farzaank
Copy link
Contributor

@farzaank farzaank commented Oct 25, 2023

Currently live at farzaank.github.io/helm

Introduces the following changes:

  • Reads config data from config.js instead of env variables to allow for version specific variables
  • Fixes routing to be consistent with current HELM site
  • Updates all links to use version based routing
  • Removes broken scenario links
  • Adds links to scenario + metric lists
  • Updates hero image with Percy's feedback
  • Updates nav dropdown to have descriptions and smaller images
  • Fixes indentation in landing page's lists
  • Adds alert on leaderboard allowing user to return to legacy results view
  • Removes jarring legacy buttons and replaces with text in about section

Future PR will make Leaderboard the landing page, and moving things around as a result

@farzaank farzaank marked this pull request as ready for review November 1, 2023 19:48
@farzaank farzaank requested a review from yifanmai November 1, 2023 19:48
Copy link
Collaborator

@yifanmai yifanmai left a comment

Choose a reason for hiding this comment

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

Looks mostly good. I'd like to merge this first so that I can try this out, but please address the suggestions in a follow-up PR.

window.BENCHMARK_OUTPUT_BASE_URL = "benchmark_output";
window.SUITE = "latest";
window.RELEASE = null;
window.LEGACY_RELEASE = "v2.4.0";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is LEGACY_RELEASE used?

@@ -3,7 +3,7 @@ export default function Card() {
<div className="stats shadow">
<div className="stat">
<div className="stat-title">Total Models</div>
<div className="stat-value">123</div>
<div className="stat-value">1</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lol.

Where is Card used?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we also add the Google Analytics tracker here (commented out)?

Comment on lines +16 to +17
{" "}
a transparent benchmarking system for language models{" "}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Delete both {" "} instead <strong>.

Comment on lines +3 to +4
window.SUITE = "v0.2.4";
window.RELEASE = "v0.3.0";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Either suite or release should be set, not both. See my other comment later on the semantics of this.

Comment on lines +48 to +49
.map((subGroup, idx) =>
subGroup.todo || subGroup.name.includes("CLEVA") ? (
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should just set todo to true for all the CLEVA results and then remove this .includes() bit.

@@ -17,5 +17,5 @@ export default defineConfig({
build: {
outDir: `${__dirname}/../helm/benchmark/static_build`,
},
base: "/helm/",
//base: "/helm/" // can't add process.env.HELM_SUITE here with GH pages
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add some documentation somewhere about how to configure and build the website with these changes e.g. does this have to change to /helm/latest/ before building?

Copy link
Collaborator

Choose a reason for hiding this comment

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

A comment in the code would be sufficient for this, for now.

if (window.RELEASE) {
return `/benchmark_output/releases/${window.RELEASE}`;
} else {
return `/benchmark_output/runs/${window.SUITE}`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to change to reflect the mechanics of the incremental release system:

JSON files pertaining to aggregations across multiple runs are stored in /benchmark_output/releases/v0.3.1/... e.g. /benchmark_output/releases/v0.3.1/groups.json

JSON files pertaining to individual runs are stored in suites /benchmark_output/runs/v0.3.0/run_name/... - but a single release can have runs from multiple suites, so there could also be runs from /benchmark_output/runs/v0.3.0/run_name/..., /benchmark_output/runs/v0.3.1/run_name/... etc.

For each run name, you need to look at /benchmark_output/runs/v0.3.0/runs_to_run_suites.json file to figure out which suite each run is from (example).

For instance, in the legacy code, we load runs_to_run_suites here, then we compute the URLs here and here.

This also means that computing the URL is an async function.

In the old code, we also supported a legacy mode, where if window.SUITE is set but window.RELEASE is not set (only one of these two will be set), then all files are stored in the suites e.g. /benchmark_output/runs/v0.3.1/groups.json, and runs_to_run_suites.json does not exist because all runs are in the same suite. I think we can punt on this for now... I'm not sure how much legacy stuff we want to support.

@yifanmai yifanmai merged commit 3648c7f into main Nov 11, 2023
3 checks passed
@yifanmai yifanmai deleted the farzaan/new-front-end branch November 11, 2023 03:48
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.

2 participants