[ML] Fixing missing multi label checkbox and basic input validation #145357

jgowdyelastic · 2022-11-16T13:39:15Z

The multi_label flag for question answering models should be user configurable.
Adds a multi label switch to the zero shot classification testing UI.

Adds basic input validation to ensure the Test button is not enabled unless all required inputs are filled in.
For fill_mask that includes every string needing contain the [MASK] keyword.

Also fixes the Test model menu item to ensure it is disabled if PyTorch models not started.

Also fixes generated pipeline to ensure it's always up to date with the user's selections.

elasticmachine · 2022-11-17T10:28:31Z

Pinging @elastic/ml-ui (:ml)

darnautov

LGTM, left a couple of suggestions

.../ml/public/application/trained_models/models_management/test_models/models/inference_base.ts

...dels/models_management/test_models/models/question_answering/question_answering_inference.ts

...ained_models/models_management/test_models/models/text_embedding/text_embedding_inference.ts

peteharverson · 2022-11-17T14:13:57Z

.../models_management/test_models/models/text_classification/zero_shot_classification_input.tsx

+        helpText={i18n.translate(
+          'xpack.ml.trainedModels.testModelsFlyout.textClassification.multiLabelHelpText',
+          {
+            defaultMessage: 'Is there a possibility of more than one correct label',


This needs a ? at the end of the sentence. Or alternatively from the docs Indicates if more than one true label is possible given the input.

Any thoughts @szabosteve ?

Yup, it's not trivial to explain it in one line. I leave a few suggestions:
Enable the input text to have more than one label.
Enable the input text to match more than one label.
What do you think?

Updated to Enable the input text to match more than one label.
13a20c0

.../ml/public/application/trained_models/models_management/test_models/models/inference_base.ts

darnautov · 2022-11-17T18:52:10Z

.../ml/public/application/trained_models/models_management/test_models/models/inference_base.ts

+  private validators$: Array<Observable<boolean>> = [];
+  private validatorsSubscriptions$: Subscription = new Subscription();
+
+  private pipeline = new BehaviorSubject<estypes.IngestPipeline>({});


I reckon we should follow the same naming convention for observables/subjects

Suggested change

private pipeline = new BehaviorSubject<estypes.IngestPipeline>({});

private pipeline$ = new BehaviorSubject<estypes.IngestPipeline>({});

darnautov · 2022-11-17T18:57:33Z

.../ml/public/application/trained_models/models_management/test_models/models/inference_base.ts

+        )
+        .subscribe((v) => {
+          this.isValid$.next(v);
+          this.pipeline.next(this.generatePipeline());


I think we should not mix validation logic with something else. Also here you generate a pipeline no matter if the input is valid or not.

If I understand it correctly, the pipeline only depends on the model id and some additional params

darnautov

Latest changes LGTM

darnautov · 2022-11-18T09:09:11Z

...ns/ml/public/application/trained_models/models_management/test_models/models/index_input.tsx

@@ -34,31 +34,35 @@ export const InferenceInputFormIndexControls: FC<Props> = ({ inferrer, data }) =
  } = data;

  const runningState = useObservable(inferrer.getRunningState$());
+  const pipeline = useObservable(inferrer.getPipeline$());


you also need an initial value

Suggested change

const pipeline = useObservable(inferrer.getPipeline$());

const pipeline = useObservable(inferrer.getPipeline$(), inferrer.getPipeline());

Updated in d70170f

darnautov · 2022-11-18T09:10:31Z

.../ml/public/application/trained_models/models_management/test_models/models/inference_base.ts

+    );
+  }
+
+  private initializePipeline(additionalChanges?: Array<Observable<any>>) {


Suggested change

private initializePipeline(additionalChanges?: Array<Observable<any>>) {

private initializePipeline(additionalChanges?: Array<Observable<unknown>>) {

Updated in 639f57b

darnautov · 2022-11-18T09:10:43Z

.../ml/public/application/trained_models/models_management/test_models/models/inference_base.ts

+  }
+
+  private initializePipeline(additionalChanges?: Array<Observable<any>>) {
+    const formObservables$: Array<Observable<any>> = [


Suggested change

const formObservables$: Array<Observable<any>> = [

const formObservables$: Array<Observable<unknown>> = [

Updated in d70170f

peteharverson

Tested latest changes and LGTM

szabosteve

UI text LGTM.

kibana-ci · 2022-11-18T13:52:08Z

💚 Build Succeeded

Buildkite Build
Commit: 48ae27a

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`ml`	3.4MB	3.4MB	+2.9KB

Unknown metric groups

ESLint disabled in files

id	before	after	diff
`osquery`	1	2	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`fleet`	59	65	+6
`osquery`	108	113	+5
`securitySolution`	442	448	+6
total			+19

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`fleet`	67	73	+6
`osquery`	109	115	+6
`securitySolution`	519	525	+6
total			+20

History

💔 Build #89160 failed 13a20c0
💔 Build #89015 failed 664d4dd
💔 Build #88953 failed 21187c8
💔 Build #88803 failed 3032eea
💚 Build #88756 succeeded 49bbd0c

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @jgowdyelastic

…lastic#145357) The `multi_label` flag for question answering models should be user configurable. Adds a multi label switch to the zero shot classification testing UI. ![image](https://user-images.githubusercontent.com/22172091/202193314-6262247b-0fef-44f0-b0d4-c62fe14e2cba.png) Adds basic input validation to ensure the Test button is not enabled unless all required inputs are filled in. For `fill_mask` that includes every string needing contain the `[MASK]` keyword. Also fixes the `Test model` menu item to ensure it is disabled if `PyTorch` models not started. ![image](https://user-images.githubusercontent.com/22172091/202516808-e48e5bc0-b7b7-4864-a94b-cec2c27b7724.png) Also fixes generated pipeline to ensure it's always up to date with the user's selections. (cherry picked from commit a929099)

kibanamachine · 2022-11-18T14:02:29Z

💚 All backports created successfully

Status	Branch	Result
✅	8.6

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine · 2022-11-22T14:04:24Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

…tion (#145357) (#145735) # Backport This will backport the following commits from `main` to `8.6`: - [[ML] Fixing missing multi label checkbox and basic input validation (#145357)](#145357)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: James Gowdy <jgowdy@elastic.co>

jgowdyelastic added 4 commits November 16, 2022 13:27

[ML] Adding multi label checkbox and basic input validation

15b5026

removing getValue

69d9471

rewriting observables

d818da9

removing comment

5c88590

jgowdyelastic requested review from darnautov and peteharverson November 17, 2022 10:26

jgowdyelastic self-assigned this Nov 17, 2022

jgowdyelastic added non-issue Indicates to automation that a pull request should not appear in the release notes :ml release_note:skip Skip the PR/issue when compiling release notes Feature:3rd Party Models ML 3rd party models v8.6.0 v8.7.0 labels Nov 17, 2022

jgowdyelastic changed the title ~~[ML] Adding multi label checkbox and basic input validation~~ [ML] Fixing missing multi label checkbox and basic input validation Nov 17, 2022

Merge branch 'main' into adding-multi-label-input-and-form-validation

9e005ff

jgowdyelastic marked this pull request as ready for review November 17, 2022 10:28

jgowdyelastic requested a review from a team as a code owner November 17, 2022 10:28

making inputs full width

49bbd0c

darnautov approved these changes Nov 17, 2022

View reviewed changes

jgowdyelastic added 2 commits November 17, 2022 13:02

moving additional validators to init function

dc96bf3

tiny change

3032eea

peteharverson reviewed Nov 17, 2022

View reviewed changes

jgowdyelastic added 2 commits November 17, 2022 17:31

fixing testable check for menu

bbf1baa

fixing pipeline updating

21187c8

darnautov requested changes Nov 17, 2022

View reviewed changes

jgowdyelastic added 3 commits November 17, 2022 20:56

refactoring observables

664d4dd

removing any type

b7e367e

tiny refactor

4822e8b

darnautov approved these changes Nov 18, 2022

View reviewed changes

jgowdyelastic added 3 commits November 18, 2022 09:42

useObservable fixes

d70170f

removing any

639f57b

uodating switch text

13a20c0

peteharverson approved these changes Nov 18, 2022

View reviewed changes

Merge branch 'main' into adding-multi-label-input-and-form-validation

48ae27a

szabosteve approved these changes Nov 18, 2022

View reviewed changes

jgowdyelastic merged commit a929099 into elastic:main Nov 18, 2022

jgowdyelastic deleted the adding-multi-label-input-and-form-validation branch November 18, 2022 13:58

kibanamachine mentioned this pull request Nov 18, 2022

[8.6] [ML] Fixing missing multi label checkbox and basic input validation (#145357) #145735

Merged

kibanamachine mentioned this pull request Nov 18, 2022

[Guided onboarding] Update codeowners for guided onboarding in the security solution #145555

Merged

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 22, 2022

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Fixing missing multi label checkbox and basic input validation #145357

[ML] Fixing missing multi label checkbox and basic input validation #145357

jgowdyelastic commented Nov 16, 2022 •

edited by kibanamachine

Loading

elasticmachine commented Nov 17, 2022

darnautov left a comment

peteharverson Nov 17, 2022

szabosteve Nov 17, 2022 •

edited

Loading

jgowdyelastic Nov 18, 2022

darnautov Nov 17, 2022

darnautov Nov 17, 2022

darnautov left a comment

darnautov Nov 18, 2022

jgowdyelastic Nov 18, 2022

darnautov Nov 18, 2022

jgowdyelastic Nov 18, 2022

darnautov Nov 18, 2022

jgowdyelastic Nov 18, 2022

peteharverson left a comment

szabosteve left a comment

kibana-ci commented Nov 18, 2022

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

kibanamachine commented Nov 18, 2022

kibanamachine commented Nov 22, 2022

	private pipeline = new BehaviorSubject<estypes.IngestPipeline>({});
	private pipeline$ = new BehaviorSubject<estypes.IngestPipeline>({});

	const pipeline = useObservable(inferrer.getPipeline$());
	const pipeline = useObservable(inferrer.getPipeline$(), inferrer.getPipeline());

	private initializePipeline(additionalChanges?: Array<Observable<any>>) {
	private initializePipeline(additionalChanges?: Array<Observable<unknown>>) {

	const formObservables$: Array<Observable<any>> = [
	const formObservables$: Array<Observable<unknown>> = [

[ML] Fixing missing multi label checkbox and basic input validation #145357

[ML] Fixing missing multi label checkbox and basic input validation #145357

Conversation

jgowdyelastic commented Nov 16, 2022 • edited by kibanamachine Loading

elasticmachine commented Nov 17, 2022

darnautov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szabosteve Nov 17, 2022 • 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

darnautov 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

peteharverson left a comment

Choose a reason for hiding this comment

szabosteve left a comment

Choose a reason for hiding this comment

kibana-ci commented Nov 18, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

kibanamachine commented Nov 18, 2022

💚 All backports created successfully

Questions ?

kibanamachine commented Nov 22, 2022

jgowdyelastic commented Nov 16, 2022 •

edited by kibanamachine

Loading

szabosteve Nov 17, 2022 •

edited

Loading