[Widgets] Refactor examples running #1023

mishig25 · 2023-10-13T13:48:35Z

Refactor codes related to running widget examples

Overall, this refactor cleans some code & adds more usage of typing.

Replaces getDemoInputs with getWidgetExample function as proposed in Proposal: Slightly better than getDemoInputs already #1021
Removes previewInputSample functions entirely by re-using applyInputSample function with new isPreview opts. (See more here)

change signature of

function applyInputSample(sample: WidgetExample)

to

function applyInputSample(sample: WidgetExample, opts: {isPreview?: boolean, inferenceOpts?: InferenceRunOpts; })

so that applyInputSample fully replaces previewInputSample when used with opts isPreview=true and applyInputSample with getWidgetExample replaces getDemoInputs and running example code in onMount. (in this example, I've used AudioClassificationWidget.svelte but the changes apply to all widgets).

Order of merge operations

Merge [Widgets] Refactor examples running #1023 (this PR)
Then merge [Widgets] Don't call api inference if output exists #1063
Then merge [Widgets] Handle display logic #1019

js/src/lib/components/InferenceWidget/shared/helpers.ts

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

SBrandeis · 2023-10-16T09:46:57Z

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

-			fileUrl = src;
-			selectedSampleUrl = src;
-			getOutput({ isOnLoadCall: true });
+		const example = getWidgetExample<WidgetExampleAssetInput<WidgetExampleOutputLabels>>(model, validateExample);


I am wondering if this logic shouldn't live in WidgetWrapper instead?
Given it's the same for every widget

I think putting this logic in WidgetWrapper onMount would be confusing.
Here is the reason: all widgets except the TextGeneration & Text2Image, have same code, which is:

if (callApiOnMount && example) { applyInputSample(example, { inferenceOpts: { isOnLoadCall: true } }); }

However, TextGeneration & Text2Image have custom logic. Which is:

const [textParam] = getSearchParams(["text"]); if (textParam) { setTextAreaValue(textParam); getOutput({ useCache: true }); } else { const example = getWidgetExample<WidgetExampleTextInput<WidgetExampleOutputText>>(model, validateExample); if (example && callApiOnMount) { applyInputSample(example, { inferenceOpts: { isOnLoadCall: true, useCache: true } }); } }

Therefore, to implement it inside WidgetWrapper onMount, we would need to check if it is TextGen or Text2Image and getSearchParams, which seems too implicit & confusing.
Wdyt?

BTW is there some kind of Svelte component inheritance? does this concept exist?

No. the closest thing in svelte is usage of slots as in

hub-docs/js/src/lib/components/InferenceWidget/shared/WidgetWrapper/WidgetWrapper.svelte

Line 118 in d2374e4

<slot name="bottom" />

maybe @coyotte508 can also take a quick look and let us know if you have ideas on whether to factor that logic in WidgetWrapper or not?

I guess we can include the onMount call in a helper and have a useExample(model, callApiOnMount, validateExample) or useExample($$props) called here.

Thought about it. I think it actually makes it look quite confusing/unclean when you have to pass 2 callback functions: validateExample & applyInputSample

so it will look like this in that case:
helpers.ts

import { onMount } from "svelte" ... export function customOnMount<T>(model: ModelData, isOnLoadCall: boolean, validate: func, applyInputSample: func){ onMount(() => { const example = getExample<T>(model, validate); if(example && isOnLoadCall){ applyInputSample(example); } }) }

and in XyzWidget.svelte

import { customOnMount } from "./helpers" ... customOnMount<WidgetOutput<T>>(model, isOnLoadCall, validateExample, applyInputSample)

If you are fine, I'm happy to merge this PR as it is and iterate afterwards

Rather than customOnMount, a useXxx name would be much better (see React-Use and Vue-use, it's a known named pattern), eg useInputSample or useLoadExample

it actually makes it look quite confusing/unclean when you have to pass 2 callback functions: validateExample & applyInputSample

You could pass $$props 😬

customOnMount<WidgetOutput<T>>

There's probably a way to not have to specify <WidgetOutput<T>>

Made an update.
All the onMount example runs are happening inside WidgetWrapper.

hub-docs/js/src/lib/components/InferenceWidget/shared/WidgetWrapper/WidgetWrapper.svelte

Line 70 in 37f2ccd

// run widget example

Specifically, there are two parts:
Part 1: run one of the examples from the model card

hub-docs/js/src/lib/components/InferenceWidget/shared/WidgetWrapper/WidgetWrapper.svelte

Lines 79 to 82 in 37f2ccd

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

if (callApiOnMount && example) {

applyInputSample(example, { inferenceOpts: { isOnLoadCall: true } });

}

Part 2: run examples from URL query param (for widgets like TextGen widget as mentioned here)

hub-docs/js/src/lib/components/InferenceWidget/shared/WidgetWrapper/WidgetWrapper.svelte

Lines 71 to 77 in 37f2ccd

if (exampleQueryParams.length) {

const example = {} as TWidgetExample;

for (const key of exampleQueryParams) {

const val = getQueryParamVal(key);

example[key] = val;

}

applyInputSample(example);

To activate, Part 2, widget (example: TextGen widget) needs to pass/activate optional prop in WudgetWrapper

hub-docs/js/src/lib/components/InferenceWidget/widgets/TextGenerationWidget/TextGenerationWidget.svelte

Line 198 in 37f2ccd

exampleQueryParams={["text"]}

@SBrandeis @coyotte508 wdyt?

at a glance it's a lot cleaner / removes a lot of code, it's nice!

WDYT @SBrandeis ?

Co-authored-by: Simon Brandeis <[email protected]>

mishig25 · 2023-10-27T09:47:43Z

find an updates in this comment

coyotte508

IMO let's pull the trigger on this PR and iterate if needed

(Nice refacto!)

SBrandeis

Cool stuff, I like the solution you've chosen
Way cleaner this way👍

Please address the few comments left before merging

js/src/lib/components/InferenceWidget/shared/WidgetWrapper/WidgetWrapper.svelte

SBrandeis · 2023-10-27T11:56:13Z

js/src/lib/components/InferenceWidget/shared/WidgetWrapper/WidgetWrapper.svelte

 	export let validateExample: (sample: WidgetExample) => sample is TWidgetExample;
+	export let exampleQueryParams: QueryParam[] = [];


Can this be typed further?

Like this for example (requires some ugly casts in onMount but I think I'm fine with it as it makes it almost impossible to pass wrong query params)

Suggested change

export let exampleQueryParams: QueryParam[] = [];

export let exampleQueryParams: (keyof TWidgetExample)[] = [];

yes, stronger typing is possible c89086b

SBrandeis · 2023-10-27T12:03:03Z

js/src/lib/components/InferenceWidget/shared/WidgetWrapper/WidgetWrapper.svelte

+			// run widget example
+			if (exampleQueryParams.length) {
+				const example = {} as TWidgetExample;
+				for (const key of exampleQueryParams) {
+					const val = getQueryParamVal(key);
+					example[key] = val;
+				}
+				applyInputSample(example);
+			} else {
+				const example = getWidgetExample<TWidgetExample>(model, validateExample);
+				if (callApiOnMount && example) {
+					applyInputSample(example, { inferenceOpts: { isOnLoadCall: true } });
+				}
+			}


If I'm not mistaken, getWidgetExample here is never called when exampleQueryParams is set in the concrete Widget component (ie most widgets)

Ok I tested it and I can confirm this behavior!

Is there anything that needs to be changed here?

If I'm not mistaken, getWidgetExample here is never called when exampleQueryParams is set in the concrete Widget component (ie most widgets)

I think this is the existing behaviour on main

Is there anything that needs to be changed here?

yes

I think this is the existing behaviour on main

No

Sorry I think my wording was not clear here. I'll try to reformulate.

On main (hf.co)

When accessing for example HuggingFaceH4/zephyr-7b-alpha, The widget picksan example at random and starts generating (expected behavior).

On this branch

The widget does not try to get the example from the widgetData and displays this error (bug?).
This is because exampleQueryParams.length is always truthy in the TextGenerationWidget, so the getWidgetExample call get skipped

Let me know if anything remains unclear 🤗

indeed, fixed the logic 7ebca52

js/src/lib/components/InferenceWidget/shared/helpers.ts

SBrandeis · 2023-10-27T12:34:46Z

js/src/lib/components/InferenceWidget/shared/WidgetWrapper/WidgetWrapper.svelte

+				if (callApiOnMount && example) {
+					applyInputSample(example, { inferenceOpts: { isOnLoadCall: true } });
+				}
+			}


Not sure but isOnLoadCall should prioritize the example output when specified, no?
Probably for a subsequent PR

mishig25 · 2023-10-30T11:09:18Z

js/src/lib/components/InferenceWidget/shared/helpers.ts

 type QueryParamVal = string | null | boolean | (string | number)[][];
 export function getQueryParamVal(key: QueryParam): QueryParamVal {
 	const searchParams = new URL(window.location.href).searchParams;
 	const value = searchParams.get(key);
-	if (["text", "context", "question", "query", "candidateLabels"].includes(key)) {
+	if (["text", "context", "question", "query", "candidate_labels"].includes(key)) {


this will not work on existing URLs of ZeroShotClassification widget with URL query candidateLabels rather than candidate_labels. However, ZeroShotClassification widgets are not used widely. Therefore, I think this breaking change is fine

I think i agree for candidate_labels (it's used in the spec for widget examples) but for structuredData it's used that way in the spec no?

https://huggingface.co/docs/hub/models-widgets-examples#structured-data-classification

wdyt #1064?
I can either merge #1064 or revert the changes back to using strucutredData. I will let you decide

see my comment there

mishig25 · 2023-10-30T11:09:43Z

js/src/lib/components/InferenceWidget/shared/helpers.ts

 		return value;
-	} else if (["table", "structuredData"].includes(key)) {
+	} else if (["table", "structured_data"].includes(key)) {


same as the comment above but for structuredData vs structured_data

https://github.com/huggingface/hub-docs/pull/1023/files/c89086b0ebea8a6968c7bc621134982270dbdbe6#r1376287272

mishig25 changed the title ~~Refactor_examples_running~~ Refactor examples running Oct 13, 2023

mishig25 mentioned this pull request Oct 13, 2023

Proposal: Slightly better than getDemoInputs already #1021

Closed

mishig25 changed the title ~~Refactor examples running~~ [Widgets] Refactor examples running Oct 15, 2023

This was referenced Oct 16, 2023

~~[Widgets] Don't call api inference if output exists~~ #1017

Closed

[Widgets] Handle display logic #1019

Merged

mishig25 added 6 commits October 16, 2023 10:50

Rm previewInputSample by reusing applyInputSample

667710d

Rm unneeded getDemoInputs function

85aac2e

better naming

c56c64f

format

aa2396e

correct typing

d2374e4

add missing await

d5325f9

mishig25 force-pushed the refactor_examples_running branch from 474d080 to d5325f9 Compare October 16, 2023 08:59

mishig25 marked this pull request as ready for review October 16, 2023 08:59

mishig25 requested review from julien-c, coyotte508, gary149, krampstudio, Pierrci and SBrandeis October 16, 2023 09:03

coyotte508 approved these changes Oct 16, 2023

View reviewed changes

SBrandeis reviewed Oct 16, 2023

View reviewed changes

js/src/lib/components/InferenceWidget/shared/helpers.ts Outdated Show resolved Hide resolved

SBrandeis reviewed Oct 16, 2023

View reviewed changes

mishig25 and others added 8 commits October 16, 2023 12:16

Update js/src/lib/components/InferenceWidget/shared/helpers.ts

2730c9c

Co-authored-by: Simon Brandeis <[email protected]>

format

3ba2298

use opts syntax

7858186

run onMount widget example inside WidgetWrapper

fd40b25

all widgets run onMount example inside WidgetWrapper

756223d

Rm previewInputSample by reusing applyInputSample

580c830

Rm unneeded getDemoInputs function

f89936c

better naming

0654367

mishig25 and others added 9 commits October 27, 2023 11:34

format

94249c4

correct typing

353f172

add missing await

23a2fe0

Update js/src/lib/components/InferenceWidget/shared/helpers.ts

c36fcdc

Co-authored-by: Simon Brandeis <[email protected]>

format

06420aa

use opts syntax

44eb4f1

run onMount widget example inside WidgetWrapper

ada2b70

all widgets run onMount example inside WidgetWrapper

37f2ccd

Merge branch 'rebase' into refactor_examples_running

8446c3e

mishig25 requested review from coyotte508 and SBrandeis October 27, 2023 09:47

coyotte508 approved these changes Oct 27, 2023

View reviewed changes

SBrandeis approved these changes Oct 27, 2023

View reviewed changes

mishig25 added 4 commits October 30, 2023 11:27

fix logic

7ebca52

stronger typing

c89086b

format

4d03786

more refactor

9ea310a

mishig25 commented Oct 30, 2023

View reviewed changes

mishig25 merged commit cd15b19 into main Oct 30, 2023
2 checks passed

mishig25 deleted the refactor_examples_running branch October 30, 2023 11:10

This was referenced Oct 30, 2023

[Widgets] Don't call api inference if output exists #1063

Merged

type WidgetExampleAttribute #1066

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Widgets] Refactor examples running #1023

[Widgets] Refactor examples running #1023

mishig25 commented Oct 13, 2023 •

edited

Loading

SBrandeis Oct 16, 2023

mishig25 Oct 16, 2023 •

edited

Loading

julien-c Oct 16, 2023

mishig25 Oct 16, 2023 •

edited

Loading

julien-c Oct 17, 2023

mishig25 Oct 19, 2023

coyotte508 Oct 19, 2023 •

edited

Loading

mishig25 Oct 27, 2023

coyotte508 Oct 27, 2023

julien-c Oct 27, 2023

mishig25 commented Oct 27, 2023

coyotte508 left a comment •

edited

Loading

SBrandeis left a comment •

edited

Loading

SBrandeis Oct 27, 2023

SBrandeis Oct 27, 2023

mishig25 Oct 30, 2023

SBrandeis Oct 27, 2023

SBrandeis Oct 27, 2023

mishig25 Oct 27, 2023 •

edited

Loading

SBrandeis Oct 27, 2023

mishig25 Oct 30, 2023

SBrandeis Oct 27, 2023

mishig25 Oct 30, 2023

julien-c Oct 30, 2023

mishig25 Oct 30, 2023

julien-c Oct 30, 2023

mishig25 Oct 30, 2023 •

edited

Loading

julien-c Oct 30, 2023

	const example = getWidgetExample<TWidgetExample>(model, validateExample);
	if (callApiOnMount && example) {
	applyInputSample(example, { inferenceOpts: { isOnLoadCall: true } });
	}

	if (exampleQueryParams.length) {
	const example = {} as TWidgetExample;
	for (const key of exampleQueryParams) {
	const val = getQueryParamVal(key);
	example[key] = val;
	}
	applyInputSample(example);

		export let validateExample: (sample: WidgetExample) => sample is TWidgetExample;
		export let exampleQueryParams: QueryParam[] = [];

	export let exampleQueryParams: QueryParam[] = [];
	export let exampleQueryParams: (keyof TWidgetExample)[] = [];

[Widgets] Refactor examples running #1023

[Widgets] Refactor examples running #1023

Conversation

mishig25 commented Oct 13, 2023 • edited Loading

Refactor codes related to running widget examples

Order of merge operations

Choose a reason for hiding this comment

mishig25 Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mishig25 Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coyotte508 Oct 19, 2023 • edited Loading

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

mishig25 commented Oct 27, 2023

coyotte508 left a comment • edited Loading

Choose a reason for hiding this comment

SBrandeis left a comment • edited Loading

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

mishig25 Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

On main (hf.co)

On this branch

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

mishig25 Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mishig25 commented Oct 13, 2023 •

edited

Loading

mishig25 Oct 16, 2023 •

edited

Loading

mishig25 Oct 16, 2023 •

edited

Loading

coyotte508 Oct 19, 2023 •

edited

Loading

coyotte508 left a comment •

edited

Loading

SBrandeis left a comment •

edited

Loading

mishig25 Oct 27, 2023 •

edited

Loading

mishig25 Oct 30, 2023 •

edited

Loading