[Widget] Refactor `WidgetWrapper` #384

mishig25 · 2023-11-29T20:32:36Z

Refactor `WidgetWrapper`

(continuing the widget rewrite)

We discussed about removing WidgetWrapper. Instead, WidgetWrapper is still kept but simplified. Specificlly, WidgetWrapper does 2 tasks: 1. common styling for all widgets (just like App inside moon); 2. gets a model status (is it loaded, loadable, or too big)
With the simplified WidgetWrapper, there is no longer WidgetHeader or WidgetFooter inside WidgetWrapper. Therefore, every widget has to explicitly call WidgetHeader or WidgetFooter.
Refactored WidgetExamples component: 1. to use less reactive statements $:; 2. have better variable names (favouring exampleXYZ over inputSampleXYZ); 3. WidgetExamples has the logic to run a random widget example onMount (previously, this logic was inside WidgetWrapper)
Removed widget maximize feature (cc: @gary149) from WidgetFooter. I don't think anyone was using. But happy to put it back.

julien-c

not related to this PR, but is there any way to generate the validators from some sort of task-specific JSONschema (one for each task)?

i.e. we'd have a single source of truth to define the shape of inputs/outputs

mishig25 · 2023-11-30T10:01:19Z

not related to this PR, but is there any way to generate the validators from some sort of task-specific JSONschema (one for each task)?

Yes. There is a lib called ajv https://github.com/ajv-validator/ajv. But I think I/we can implement something simple/small in our use case.

i.e. we'd have a single source of truth to define the shape of inputs/outputs

indeed, that would be great. Can also be used to auto generate the docs page https://huggingface.co/docs/hub/models-widgets-examples#widget-examples

coyotte508 · 2023-11-30T10:49:13Z

maybe after we switch to using @huggingface/inference? Since the vlaidation is also done there

julien-c · 2023-11-30T11:18:16Z

There is a lib called ajv

lol, going full circle (we used to use ajv before switching to Joi)

maybe after we switch to using @huggingface/inference? Since the vlaidation is also done there

Yes why not, and anyways the single source of truth's idea is that it would also be used in our Python libraries (cc @Wauplin)

switch to using @huggingface/inference

how complex/disruptive would this be? (and what the other advantages?)

mishig25 · 2023-11-30T11:20:11Z

maybe after we switch to using @huggingface/inference? Since the vlaidation is also done there

@huggingface/inference validates api-output. Does it also validate input to the api? Also, we need to validate widget inputs and outputs (which can differ from inference input/output). For instance, on Text2Img widget, inference output is fileBlob while example output is url to file

coyotte508 · 2023-11-30T11:22:09Z

how complex/disruptive would this be? (and what the other advantages?)

The main advantage is putting in common the options (eg regarding cache, model loading) + making sure @huggingface/inference is up-to-date / everything's compatible

coyotte508 · 2023-11-30T11:26:00Z

@huggingface/inference validates api-output. Does it also validate input to the api?

No (and no plan to), we rely on the type system for that.

coyotte508

Nice! I love that we pass a lot less props to WidgetWrapper each time.

Two main comments:

about calling getContext at top-level
about form

The rest you are free to ignore if you want ^^

PS: we should probably fix the Icon links (lots of //) but that comes from an earlier PR

packages/widgets/src/lib/components/InferenceWidget/shared/WidgetWrapper/WidgetWrapper.svelte

packages/widgets/src/lib/components/InferenceWidget/shared/WidgetHeader/WidgetHeader.svelte

...omponents/InferenceWidget/widgets/AudioClassificationWidget/AudioClassificationWidget.svelte

coyotte508

Thank you!

SBrandeis · 2023-12-05T14:08:47Z

Reviewing this now

SBrandeis

Nice refactor.
I haven't tested all the widgets.
I was a bit confused at first by the examples logic living in WidgetHeader, but that's probably just a wording issue.

SBrandeis · 2023-12-05T14:34:06Z

packages/widgets/src/lib/components/InferenceWidget/shared/WidgetFooter/WidgetFooter.svelte

-		{:else}
-			Minimize
-		{/if}
-	</button>


What is the rationale for removing the maximize feature?

I can put it back. I thought that this feature was not being used anywhere (point 4)

I'm not sure, I would have kept it as it was 👀

handled in edb463b

packages/widgets/src/lib/components/InferenceWidget/shared/WidgetHeader/WidgetHeader.svelte

packages/widgets/src/lib/components/InferenceWidget/shared/helpers.ts

SBrandeis · 2023-12-05T15:33:44Z

packages/widgets/src/lib/components/InferenceWidget/shared/WidgetExamples/WidgetExamples.svelte

+				applyWidgetExample(exampleFromQueryParams);
+			} else {
+				// run random widget example
+				const example = getWidgetExample<TWidgetExample>(model, validateExample);


I think we should use allExamples here no?
It's already extracted from the widget and validated
Will allow us to remove the validateExample & model props from this component too

Suggested change

const example = getWidgetExample<TWidgetExample>(model, validateExample);

const example = randomItem(allExamples);

handled as part of f22f63e

SBrandeis · 2023-12-05T15:40:12Z

packages/widgets/src/lib/components/InferenceWidget/shared/WidgetHeader/WidgetHeader.svelte

+	export let applyWidgetExample: (sample: TWidgetExample, opts?: ExampleRunOpts) => void = () => {};
+	export let validateExample: (sample: WidgetExample) => sample is TWidgetExample = (
+		sample
+	): sample is TWidgetExample => true;


A bit confused by those defaults, maybe safer to have undefined as a default?

handled as part of f22f63e

SBrandeis · 2023-12-05T15:41:54Z

packages/widgets/src/lib/components/InferenceWidget/shared/WidgetHeader/WidgetHeader.svelte

+		// if there are no examples with outputs AND model.inference !== InferenceDisplayability.Yes
+		// then widget will show InferenceDisplayability error to the user without showing anything else
+		if (isDisabled && !examples.length) {
+			$widgetNoInference[model.id] = true;


Unrelated to the current PR: do stores get persisted when refreshing the page?

What happens when the store value changes? (Are the consuming components getting updated?)

do stores get persisted when refreshing the page?

Svelte stores don't persist.

What happens when the store value changes? (Are the consuming components getting updated?)

if a consuming component had UI or reactive statement dependent on a store, that UI or reactive statement gets updated when store gets a update

SBrandeis · 2023-12-05T15:46:18Z

...omponents/InferenceWidget/widgets/AudioClassificationWidget/AudioClassificationWidget.svelte

+	let:isDisabled
+	let:modelLoadInfo
+	let:WidgetInfo
+	let:WidgetHeader
+	let:WidgetFooter


Maybe it would be simpler / cleaner if isDisabled / modelLoadInfo were in a store?
From what I understand they are only exposed to be "re-injected" in WidgetHeader / WidgetInfo, correct?

handled in 9861a29

…pers.ts Co-authored-by: Simon Brandeis <33657802+SBrandeis@users.noreply.github.com>

…getHeader/WidgetHeader.svelte Co-authored-by: Simon Brandeis <33657802+SBrandeis@users.noreply.github.com>

mishig25 · 2023-12-05T16:29:41Z

I was a bit confused at first by the examples logic living in WidgetHeader, but that's probably just a wording issue.

update: commit f22f63e

Worked bit more on wording and var names

WidgetExamples component is inside WidgetHeader. And there is a widget examples logic inside WidgetHeader that does:

<script>
...
const validExamples = areThereValidWidgetExamples()
...
</script>

<div>
{if validExamples}
<WidgetExamples>
{/if}
</div>

SBrandeis

Ok LGTM
Sorry for the delay in reviewing
Note that different widgets for the same model on the same page will share some state due to stores

mishig25 marked this pull request as draft November 29, 2023 20:32

mishig25 changed the title ~~Refactor WidgetWrapper~~ [Widget] Refactor WidgetWrapper Nov 29, 2023

mishig25 force-pushed the widgets_refactor_5 branch from 7bcd57d to ac6faf3 Compare November 29, 2023 20:53

mishig25 marked this pull request as ready for review November 29, 2023 21:01

mishig25 requested review from SBrandeis, coyotte508, gary149, julien-c and osanseviero November 29, 2023 21:01

julien-c reviewed Nov 30, 2023

View reviewed changes

coyotte508 mentioned this pull request Nov 30, 2023

🦺 Validator util for hf/inference #385

Draft

coyotte508 reviewed Nov 30, 2023

View reviewed changes

mishig25 added 4 commits December 5, 2023 14:08

Refactor WidgetWrapper

d9888a5

Rn applyInputSample -> applyWidgetExample

eb8e24b

Fix <form> usage

9628f72

Use store over context

adaea0b

mishig25 force-pushed the widgets_refactor_5 branch from ff12e1c to adaea0b Compare December 5, 2023 13:19

mishig25 requested a review from coyotte508 December 5, 2023 13:20

format

5eb0a9d

coyotte508 approved these changes Dec 5, 2023

View reviewed changes

SBrandeis reviewed Dec 5, 2023

View reviewed changes

Mishig and others added 2 commits December 5, 2023 08:10

Update packages/widgets/src/lib/components/InferenceWidget/shared/hel…

a587bea

…pers.ts Co-authored-by: Simon Brandeis <33657802+SBrandeis@users.noreply.github.com>

Update packages/widgets/src/lib/components/InferenceWidget/shared/Wid…

5c4ddcc

…getHeader/WidgetHeader.svelte Co-authored-by: Simon Brandeis <33657802+SBrandeis@users.noreply.github.com>

mishig25 added 2 commits December 5, 2023 17:18

fix git apply suggestion

19650ac

rm unused import

326aa5a

mishig25 added 6 commits December 6, 2023 10:30

Use stores for ModelLoadInfo & isDisabled

9861a29

em unused import

38708d1

bring maximize functionality back

edb463b

refactor examples & examples validation

f22f63e

fxi ci check

970dcb4

rm unused imports

6648655

mishig25 requested review from a team and SBrandeis December 6, 2023 11:13

coyotte508 mentioned this pull request Dec 8, 2023

Use @huggingface/inference inside @huggingface/widgets #360

Closed

🩹 test page: use mistral instead of gpt2

af91656

SBrandeis approved these changes Dec 13, 2023

View reviewed changes

mishig25 merged commit 3782a46 into main Dec 18, 2023

mishig25 deleted the widgets_refactor_5 branch December 18, 2023 15:02

	const example = getWidgetExample<TWidgetExample>(model, validateExample);
	const example = randomItem(allExamples);

[Widget] Refactor WidgetWrapper #384

[Widget] Refactor WidgetWrapper #384

Uh oh!

Conversation

mishig25 commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Refactor WidgetWrapper

Uh oh!

julien-c left a comment

Choose a reason for hiding this comment

Uh oh!

mishig25 commented Nov 30, 2023

Uh oh!

coyotte508 commented Nov 30, 2023

Uh oh!

julien-c commented Nov 30, 2023

Uh oh!

mishig25 commented Nov 30, 2023

Uh oh!

coyotte508 commented Nov 30, 2023

Uh oh!

coyotte508 commented Nov 30, 2023

Uh oh!

coyotte508 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coyotte508 left a comment

Choose a reason for hiding this comment

Uh oh!

SBrandeis commented Dec 5, 2023

Uh oh!

SBrandeis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mishig25 Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mishig25 commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SBrandeis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[Widget] Refactor `WidgetWrapper` #384

[Widget] Refactor `WidgetWrapper` #384

mishig25 commented Nov 29, 2023 •

edited

Loading

Refactor `WidgetWrapper`

mishig25 Dec 5, 2023 •

edited

Loading

mishig25 commented Dec 5, 2023 •

edited

Loading