Initial React Frontend #1819

mkly · 2023-08-29T18:13:19Z

Add initial react frontend to be deployed via github pages.
Include GitHub action to deploy site.
Base is set to /helm/ to align with current deployment
Environment variables will need to be set in CI:
- VITE_HELM_BENCHMARKS_ENDPOINT
- VITE_HELM_BENCHMARKS_SUITE
Routes make use of HashRouter

* Add initial react frontend to be deployed via github pages. * Include GitHub action to deploy site. * Base is set to `/helm/` to align with current deployment * Enviroment variables will need to be set in CI: - VITE_HELM_BENCHMARKS_ENDPOINT - VITE_HELM_BENCHMARKS_SUITE * Routes make use of `HashRouter`

mkly · 2023-08-29T18:17:26Z

Can be viewed on the forked branch here:
https://mkly.github.io/helm/

yifanmai · 2023-08-29T18:31:42Z

This is awesome, thank you! I'll send you a review in a day or two :)

percyliang · 2023-08-29T21:02:30Z

Great work - thank you!

yifanmai

Sorry this is taking me longer to review than expected, but I left some initial comments first.

src/helm-frontend/src/components/MardownValue.test.tsx

src/helm-frontend/src/components/MetricsList.tsx

src/helm-frontend/src/types/GroupMetadata.ts

src/helm-frontend/src/types/Metric.ts

yifanmai · 2023-09-05T18:23:24Z

src/helm-frontend/src/App.css

+@tailwind utilities;
+
+/**
+ * @TODO move me to component


Could you move this to the components now?

This one is a bit of an odd one. I added that comment originally for a reminder to look into potentially moving those styles to the components and neglected to remove it. I was unable to uncover a method to do that without introducing significant additional complexity. I am open to suggestions you may have as it is very possible I am not aware of ways to make that happen.

Sounds good, I'm fine with having this be here. We don't have very much component-specific CSS anyway.

src/helm-frontend/README.md

.github/workflows/frontend.yml

yifanmai · 2023-09-06T00:14:45Z

src/helm-frontend/tsconfig.json

+    "types": ["vitest/globals", "vite/client"]
+  },
+  "include": ["src"],
+  "references": [{ "path": "./tsconfig.node.json" }]


I'm a bit confused how the references field here works, and also the include field in tsconfig.node.json... could you give a quick explanation or link for my benefit? Or is this auto-generated scaffolding?

This does appear to be part of the template when creating the project:

https://github.com/vitejs/vite/blob/ab5c421571485df861119e1f2366a84cc12b212d/packages/create-vite/template-react-ts/tsconfig.json#L24

I am not sure why the node specific config needs to referenced.

Ah, if it's auto-generated, then it should be okay. Maybe I will investigate further later.

yifanmai · 2023-09-06T00:15:42Z

src/helm-frontend/src/types/Stat.ts

+      prob: number;
+      robustness: boolean;
+      source_class: string;
+      target_class: string;


I'm not sure if TypeScript supports this, but perturbation could contain extra fields of arbitrary type.

Also I think this definition can be combined with the Perturbation.ts earlier.

I admittedly did a bit of best guessing with some of these types. Were you thinking about something like this?

export default interface Perturbation { [key: string]: string | number | boolean | undefined; name?: string; display_name?: string; description?: string; computed_on?: string; fairness?: boolean; name_file_path?: string; person_name_type?: string; preserve_gender?: boolean; prob?: number; robustness?: boolean; source_class?: string; target_class?: string; }

And then using that type in types/Stat?

That would have the benefit of still providing some additional hinting within the IDE, while still allowing for additional values.

In a related note, I did notice that there are several typos where Perturbation is written as Peturbation. I will update that once we align on the best path forward here.

Yes, I think [key: string]: string | number | boolean | undefined; is a good idea. And it would be a good idea to resolve the typos as well.

mkly · 2023-09-06T00:27:22Z

No problem! Thank you for the feedback and I'll address those for tomorrow.

* Fix MarkdownValue test filename * Correct semantic markdown on homepage lists * Fix `GroupMetaData` typo * Fix `IMetric` typo * Remove incorrect readme info about pre-built static files * Add `paths` filter to only test and build within `src/helm-frontend`

* Remove incorrect readme info about pre-built static files (this was missed in previous commit)

…tend

yifanmai

Sorry for the delay, I got pre-empted by other things. Almost done with the review; only have a few components left to look through, which I should be able to finish up tomorrow.

yifanmai · 2023-09-20T22:52:12Z

src/helm-frontend/src/services/getDisplayPredictionsByName.ts

+
+    return await displayPrediction.json() as DisplayPrediction[];
+  } catch (error) {
+    console.log(error);


console.error instead of console.log (same for the other services).

yifanmai · 2023-09-21T00:00:50Z

src/helm-frontend/src/App.css

+@tailwind utilities;
+
+/**
+ * @TODO move me to component


Sounds good, I'm fine with having this be here. We don't have very much component-specific CSS anyway.

yifanmai · 2023-09-21T00:01:19Z

src/helm-frontend/tsconfig.json

+    "types": ["vitest/globals", "vite/client"]
+  },
+  "include": ["src"],
+  "references": [{ "path": "./tsconfig.node.json" }]


Ah, if it's auto-generated, then it should be okay. Maybe I will investigate further later.

yifanmai · 2023-09-21T00:02:00Z

src/helm-frontend/src/types/Stat.ts

+      prob: number;
+      robustness: boolean;
+      source_class: string;
+      target_class: string;


Yes, I think [key: string]: string | number | boolean | undefined; is a good idea. And it would be a good idea to resolve the typos as well.

yifanmai

I think this is nearly ready for merging; just have some minor stuff left. I'm okay with punting some stuff to future PRs, if desired.

yifanmai · 2023-09-21T23:24:57Z

src/helm-frontend/src/components/PreView.tsx

+  value: string;
+}
+
+export default function PreView({ value }: Props) {


Nit: Preview instead of PreView (same for file name)

yifanmai · 2023-09-21T23:27:23Z

src/helm-frontend/src/components/ScenariosList.tsx

+              {subGroups.filter((subGroup) =>
+                (topLevelGroup.subgroups || []).includes(subGroup.name)
+              ).map((subGroup, idx) => (
+                subGroup.todo


nit: can just do const classNames = subGroup.todo ? "ml-4 text-slate-300" : "ml-4" since the rest of the element is the same

yifanmai · 2023-09-28T20:58:57Z

Hi Mike, I am thinking of merging the current as is, because this would allow other folks to start playing around with the React Frontend and building on top of it. The remaining suggestions can be resolved in a subsequent PR. WDYT?

Fix typos and cleanup * Fix naming typos * Update npm package for security warning * Remove leftover comments

mkly · 2023-10-01T00:15:21Z

I pushed some additional adjustments based on the review discussion. Please feel free to merge this in at any time. Thank you again for all your time to review the work.

mkly · 2023-10-01T00:20:47Z

As a reminder, this line will need to be adjusted in the CI/CD Github action for moving to the main branch for deployment. I did not change it because if I did this branch would no build the example site for review.

https://github.com/stanford-crfm/helm/pull/1819/files#diff-ffe3a297ed15e9a79846161a3731a580511bf13c6b5cc5a9e03ddcbd8515fdeeR38-R39

yifanmai · 2023-10-03T17:16:33Z

Sounds good, I will change the branch name for CI/CD in a separate PR. Thanks again!

yifanmai · 2023-10-03T17:21:51Z

Merged - and this is one of the biggest PRs we've merged so far. Congrats and thanks again @mkly !

percyliang · 2023-10-03T17:50:17Z

Thank you for the great work, @mkly !

yifanmai requested changes Sep 6, 2023

View reviewed changes

mkly added 3 commits September 7, 2023 21:47

Review fixes

409d8d3

* Fix MarkdownValue test filename * Correct semantic markdown on homepage lists * Fix `GroupMetaData` typo * Fix `IMetric` typo * Remove incorrect readme info about pre-built static files * Add `paths` filter to only test and build within `src/helm-frontend`

Review fixes

9b3686c

* Remove incorrect readme info about pre-built static files (this was missed in previous commit)

Merge branch 'react_frontend' of github.com:mkly/helm into react_fron…

95b6571

…tend

yifanmai requested changes Sep 21, 2023

View reviewed changes

yifanmai requested changes Sep 22, 2023

View reviewed changes

Fix typos and cleanup

a8896d4

Fix typos and cleanup * Fix naming typos * Update npm package for security warning * Remove leftover comments

yifanmai approved these changes Oct 3, 2023

View reviewed changes

yifanmai merged commit d3a0cc1 into stanford-crfm:main Oct 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial React Frontend #1819

Initial React Frontend #1819

mkly commented Aug 29, 2023

mkly commented Aug 29, 2023

yifanmai commented Aug 29, 2023

percyliang commented Aug 29, 2023

yifanmai left a comment

yifanmai Sep 5, 2023

mkly Sep 8, 2023

yifanmai Sep 21, 2023

yifanmai Sep 6, 2023

mkly Sep 8, 2023

yifanmai Sep 21, 2023

yifanmai Sep 6, 2023

mkly Sep 8, 2023

yifanmai Sep 21, 2023

mkly commented Sep 6, 2023

yifanmai left a comment

yifanmai Sep 20, 2023

yifanmai Sep 21, 2023

yifanmai Sep 21, 2023

yifanmai Sep 21, 2023

yifanmai left a comment

yifanmai Sep 21, 2023

yifanmai Sep 21, 2023

yifanmai commented Sep 28, 2023

mkly commented Oct 1, 2023

mkly commented Oct 1, 2023

yifanmai commented Oct 3, 2023

yifanmai commented Oct 3, 2023

percyliang commented Oct 3, 2023

Initial React Frontend #1819

Initial React Frontend #1819

Conversation

mkly commented Aug 29, 2023

mkly commented Aug 29, 2023

yifanmai commented Aug 29, 2023

percyliang commented Aug 29, 2023

yifanmai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkly commented Sep 6, 2023

yifanmai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yifanmai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yifanmai commented Sep 28, 2023

mkly commented Oct 1, 2023

mkly commented Oct 1, 2023

yifanmai commented Oct 3, 2023

yifanmai commented Oct 3, 2023

percyliang commented Oct 3, 2023