[ML] Add indicator if there are stopped partitions in categorization job wizard #75709

qn895 · 2020-08-21T22:07:34Z

Summary

Part of #73968 to add an indicator in the categorization job wizard if the categorization status goes to warn for at least one partition. It will only perform the check if analysis_config.per_partition_categorization.stop_on_warn is set to True

Currently, it's showing a preview of 5 stopped partitions maximum, where after it reaches that amount, will stop making calls to retrieve the list.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
This was checked for cross-browser compatibility

elasticmachine · 2020-08-21T22:07:36Z

Pinging @elastic/ml-ui (:ml)

qn895 · 2020-08-21T22:08:47Z

Alternatively, we can also keep fetching the updated list of partitions and show a more accurate count, while keeping the preview small.

...s/components/pick_fields_step/components/categorization_view/category_stopped_partitions.tsx

peteharverson

Tested and looks good. Just added a couple of minor suggestions.

...s/components/pick_fields_step/components/categorization_view/category_stopped_partitions.tsx

…d-partitions-preview

jgowdyelastic · 2020-08-25T09:10:10Z

...s/components/pick_fields_step/components/categorization_view/category_stopped_partitions.tsx

+      // no need to keep fetching anymore
+      if (
+        stoppedPartitionsPreview.length >= NUMBER_OF_PREVIEW &&
+        _resultsSubscription !== undefined


can _resultsSubscription.closed be used here?

jgowdyelastic · 2020-08-25T10:23:38Z

...s/components/pick_fields_step/components/categorization_view/category_stopped_partitions.tsx

+    // only need to run this check if jobCreator.perPartitionStopOnWarn is turned on
+    if (jobCreator.perPartitionCategorization && jobCreator.perPartitionStopOnWarn) {
+      // subscribe to result updates
+      _resultsSubscription = resultsLoader.subscribeToResults(setResultsWrapper);


keeping the subscription stored in _resultsSubscription which is not in a hook will mean it is redeclared on every render.

loadCategoryStoppedPartitions could be moved inside this useEffect or hasStoppedPartitions could be changed to stoppedPartitionsCount which blocks the running of the check, rather than unsubscribing early.
I think the counter change will be the simplest.

I ended up using a ref 7a256b6 to make sure _resultsSubscription is not re-declared every time the component renders. Although I can also expose out this._results$ from resultsLoader as well so we can take advantage of other util observable functions if that's better in the long run.

jgowdyelastic · 2020-08-25T10:36:15Z

...s/components/pick_fields_step/components/categorization_view/category_stopped_partitions.tsx

+  }
+
+  async function loadCategoryStoppedPartitions() {
+    const results = await ml.results.getCategoryStoppedPartitions([jobCreator.jobId]);


This fires when the job doesn't yet exist, which is fine, but it results in an uncaught error.
This error should be caught. if it's a 404 it can be ignored.

Updated here 676dfff

…d-partitions-preview

jgowdyelastic · 2020-08-26T10:35:45Z

...s/components/pick_fields_step/components/categorization_view/category_stopped_partitions.tsx

+    if (
+      jobCreator.perPartitionCategorization &&
+      jobCreator.perPartitionStopOnWarn &&
+      _resultsSubscription?.current


this check stops the subscription from being created because _resultsSubscription?.current is null initially

Thanks. I removed this whole block and replaced it with the rxjs utils instead here 63a4e02

...s/components/pick_fields_step/components/categorization_view/category_stopped_partitions.tsx

darnautov · 2020-08-26T14:28:04Z

...s/components/pick_fields_step/components/categorization_view/category_stopped_partitions.tsx

+      _resultsSubscription?.current
+    ) {
+      // subscribe to result updates
+      _resultsSubscription.current = resultsLoader.subscribeToResults(setResultsWrapper);


I'm not sure if you need a reference for the subscription

You could try to utilize taleUntil or takeWhile operators to stop fetching

Suggested change

_resultsSubscription.current = resultsLoader.subscribeToResults(setResultsWrapper);

resultsLoader.pipe(

switchMap(() => {

// fetch results

return from(ml.results.getCategoryStoppedPartitions([jobCreator.jobId]))

}),

tap(r => {

// update your state here

}),

takeWhile(r => {

return stoppedPartitionsPreview.length , NUMBER_OF_PREVIEW

}

)

Thanks I have updated here 63a4e02

…d-partitions-preview

jgowdyelastic

Added a couple of small comments but generally LGTM

jgowdyelastic · 2020-08-27T07:18:17Z

...s/components/pick_fields_step/components/categorization_view/category_stopped_partitions.tsx

+  const [tableRow, setTableRow] = useState<Array<{ partitionName: string }>>([]);
+  const [stoppedPartitionsError, setStoppedPartitionsError] = useState<string | undefined>();
+
+  // using ref so this Subscription instance is only intialized once


this comment isn't needed anymore

Removed here cc44727

jgowdyelastic · 2020-08-27T07:18:58Z

...s/components/pick_fields_step/components/categorization_view/category_stopped_partitions.tsx

+
+  const loadCategoryStoppedPartitions = useCallback(async () => {
+    try {
+      const results = await ml.results.getCategoryStoppedPartitions([jobCreator.jobId]);


nit, this could be { jobs } rather than results

Updated here cc44727

darnautov · 2020-08-27T07:59:34Z

...s/components/pick_fields_step/components/categorization_view/category_stopped_partitions.tsx

+      const resultsSubscription = resultsLoader.results$
+        .pipe(
+          switchMap(() => {
+            return from(loadCategoryStoppedPartitions());


loadCategoryStoppedPartitions depends on jobCreator.jobId, but your useEffect is only executed once, hence it only gets a reference of the initial callback. It could lead to unexpected behavior.

from my testing jobId is always correct here as this component is recreated when navigating to the summary step and the jobId cannot be changed by the user without navigating away to a previous step.

...s/components/pick_fields_step/components/categorization_view/category_stopped_partitions.tsx

darnautov · 2020-08-27T11:24:54Z

...s/components/pick_fields_step/components/categorization_view/category_stopped_partitions.tsx

+          takeWhile((results) => {
+            return !results || (Array.isArray(results) && results.length <= NUMBER_OF_PREVIEW);
+          })


When I shared a pseudo-code example I missed one point - takeWhile operator should be placed before switchMap where you perform stopped partitions fetching. With a current implementation potentially the stream can perform an unnecessary API call. It is not critical, but worth mentioning. 🙂 Can be improved later.

darnautov

Overall code LGTM. Worth extracting the observable part to a hook/function to cover it with some unit tests, to make sure it doesn't perform unnecessary API calls and closes the stream accordingly. You can check out model_memory_estimator tests as example.

peteharverson

Tested latest edits and LGTM

…d-partitions-preview

kibanamachine · 2020-08-27T18:23:12Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: cc44727

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
ml	1374	+1	1373

async chunks size

id	value	diff	baseline
ml	8.2MB	+15.0KB	8.2MB

distributable file count

id	value	diff	baseline
total	53127	+1	53126

History

💚 Build #70696 succeeded 27445b0
💚 Build #70392 succeeded 39bcf77
💚 Build #70159 succeeded 618fa8f
💔 Build #69831 failed cf460be

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

…job wizard (elastic#75709)

…ation job wizard (#75709) (#76137)

[ML] Add preview for stopped_partitions in categorization job wizard

cf460be

qn895 added release_note:enhancement :ml Feature:Anomaly Detection ML anomaly detection v8.0.0 v7.10.0 labels Aug 21, 2020

qn895 requested review from alvarezmelissa87, jgowdyelastic and peteharverson August 21, 2020 22:07

qn895 requested a review from a team as a code owner August 21, 2020 22:07

qn895 self-assigned this Aug 21, 2020

peteharverson reviewed Aug 24, 2020

View reviewed changes

...s/components/pick_fields_step/components/categorization_view/category_stopped_partitions.tsx Outdated Show resolved Hide resolved

peteharverson reviewed Aug 24, 2020

View reviewed changes

...s/components/pick_fields_step/components/categorization_view/category_stopped_partitions.tsx Show resolved Hide resolved

...s/components/pick_fields_step/components/categorization_view/category_stopped_partitions.tsx Outdated Show resolved Hide resolved

qn895 added 2 commits August 24, 2020 15:48

Merge remote-tracking branch 'upstream/master' into job-wizard-stoppe…

0a53e43

…d-partitions-preview

[ML] Update types, change message, add spacer

618fa8f

jgowdyelastic reviewed Aug 25, 2020

View reviewed changes

qn895 added 5 commits August 25, 2020 10:19

Merge remote-tracking branch 'upstream/master' into job-wizard-stoppe…

268c13f

…d-partitions-preview

[ML] Make sure _resultsSubscription is only initialized once

7a256b6

[ML] Update callout message & plurality

676dfff

Merge remote-tracking branch 'upstream/master' into job-wizard-stoppe…

eb14fc9

…d-partitions-preview

[ML] Update type

39bcf77

jgowdyelastic reviewed Aug 26, 2020

View reviewed changes

darnautov reviewed Aug 26, 2020

View reviewed changes

...s/components/pick_fields_step/components/categorization_view/category_stopped_partitions.tsx Outdated Show resolved Hide resolved

darnautov reviewed Aug 26, 2020

View reviewed changes

qn895 added 3 commits August 26, 2020 15:51

[ML] Update subscriptions

63a4e02

Merge remote-tracking branch 'upstream/master' into job-wizard-stoppe…

e282831

…d-partitions-preview

[ML] Always show <= 5

27445b0

qn895 requested review from darnautov and jgowdyelastic August 26, 2020 21:03

jgowdyelastic approved these changes Aug 27, 2020

View reviewed changes

darnautov reviewed Aug 27, 2020

View reviewed changes

...s/components/pick_fields_step/components/categorization_view/category_stopped_partitions.tsx Outdated Show resolved Hide resolved

darnautov reviewed Aug 27, 2020

View reviewed changes

...s/components/pick_fields_step/components/categorization_view/category_stopped_partitions.tsx Outdated Show resolved Hide resolved

darnautov reviewed Aug 27, 2020

View reviewed changes

darnautov approved these changes Aug 27, 2020

View reviewed changes

peteharverson approved these changes Aug 27, 2020

View reviewed changes

qn895 added 2 commits August 27, 2020 11:17

Merge remote-tracking branch 'upstream/master' into job-wizard-stoppe…

ffd49c2

…d-partitions-preview

[ML] Remove redundancy

cc44727

qn895 merged commit 1bd8f41 into elastic:master Aug 27, 2020

qn895 deleted the job-wizard-stopped-partitions-preview branch August 27, 2020 18:50

qn895 mentioned this pull request Aug 27, 2020

[7.x] [ML] Add indicator if there are stopped partitions in categorization job wizard (#75709) #76137

Merged

qn895 added a commit to qn895/kibana that referenced this pull request Aug 27, 2020

[ML] Add indicator if there are stopped partitions in categorization …

238f627

…job wizard (elastic#75709)

qn895 added a commit that referenced this pull request Aug 28, 2020

[7.x] [ML] Add indicator if there are stopped partitions in categoriz…

9f630a2

…ation job wizard (#75709) (#76137)

-      _resultsSubscription.current = resultsLoader.subscribeToResults(setResultsWrapper);
+   resultsLoader.pipe(
+     switchMap(() => {
+       // fetch results
+       return from(ml.results.getCategoryStoppedPartitions([jobCreator.jobId]))
+     }),
+     tap(r => {
+     // update your state here
+     }),
+     takeWhile(r => {
+       return stoppedPartitionsPreview.length , NUMBER_OF_PREVIEW
+     }
+   )

[ML] Add indicator if there are stopped partitions in categorization job wizard #75709

[ML] Add indicator if there are stopped partitions in categorization job wizard #75709

Uh oh!

Conversation

qn895 commented Aug 21, 2020

Summary

Checklist

Uh oh!

elasticmachine commented Aug 21, 2020

Uh oh!

qn895 commented Aug 21, 2020

Uh oh!

Uh oh!

peteharverson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgowdyelastic Aug 25, 2020 • 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!

qn895 Aug 26, 2020 • 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!

jgowdyelastic Aug 25, 2020 • 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!

Choose a reason for hiding this comment

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!

jgowdyelastic left a comment

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darnautov left a comment

Choose a reason for hiding this comment

Uh oh!

peteharverson left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Aug 27, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

distributable file count

History

Uh oh!

jgowdyelastic Aug 25, 2020 •

edited

Loading

qn895 Aug 26, 2020 •

edited

Loading

jgowdyelastic Aug 25, 2020 •

edited

Loading