Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ML] Do not init model memory estimation with the default value #61589

Merged
merged 24 commits into from
Apr 3, 2020

Conversation

darnautov
Copy link
Contributor

@darnautov darnautov commented Mar 27, 2020

Summary

Follow up on #60888.
Prevent the model memory estimator to emit the default value on subscription start.

Checklist

@darnautov darnautov added :ml Feature:Anomaly Detection ML anomaly detection v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.8.0 labels Mar 27, 2020
@darnautov darnautov requested a review from a team as a code owner March 27, 2020 09:46
@darnautov darnautov self-assigned this Mar 27, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally and LGTM

@pheyos
Copy link
Member

pheyos commented Mar 27, 2020

Verifying test stability in https://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/308/ ...

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

: of(DEFAULT_MODEL_MEMORY_LIMIT);
tap(v => {
// eslint-disable-next-line no-console
console.log('New config: ', JSON.stringify(v, null, 2));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these log messages are adding a lot of information to the console. are they still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still need them for checking the flaky test runner, will remove after

@jgowdyelastic jgowdyelastic self-requested a review April 1, 2020 14:29
@darnautov darnautov requested a review from pheyos April 1, 2020 17:11
@darnautov
Copy link
Contributor Author

retest

1 similar comment
@darnautov
Copy link
Contributor Author

retest

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested latest edits and LGTM

Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pheyos
Copy link
Member

pheyos commented Apr 2, 2020

It works fine on my local machine. In order to check that this PR doesn't introduce test flakiness, we're validating it in 40 runs here:
https://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/332/

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@pheyos
Copy link
Member

pheyos commented Apr 3, 2020

ML tests passed 40 times in https://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/332/, so it seems no test flakiness has been introduced ✔️

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@darnautov darnautov merged commit 4fb4a71 into elastic:master Apr 3, 2020
@darnautov darnautov deleted the ML-60386-change-default branch April 3, 2020 11:45
darnautov added a commit to darnautov/kibana that referenced this pull request Apr 3, 2020
…tic#61589)

* [ML] do not init model memory estimator with the default value

* [ML] enhance model_memory_estimator logic, update unit tests

* [ML] don't call the endpoint when start the job cloning

* [ML] unit tests

* [ML] use skip

* [ML] remove unused parameter

* [ML] try to disable 'disable-dev-shm-usage'

* [ML] revert webdriver.ts, add debug logging

* [ML] add debug logging

* [ML] fix time range initialization

* [ML] fix with useMemo

* [ML] fix categorization validation check

* [ML] remove wrong setIsWizardReady

* [ML] revert page.tsx, update model_memory_estimator.ts and tests, skip failing tests

* [ML] adjust unit test description

* [ML] fix _runAdvancedValidation

* [ML] support async validation init of categorization job creator

* [ML] adjust unit tests
darnautov added a commit that referenced this pull request Apr 3, 2020
…) (#62430)

* [ML] do not init model memory estimator with the default value

* [ML] enhance model_memory_estimator logic, update unit tests

* [ML] don't call the endpoint when start the job cloning

* [ML] unit tests

* [ML] use skip

* [ML] remove unused parameter

* [ML] try to disable 'disable-dev-shm-usage'

* [ML] revert webdriver.ts, add debug logging

* [ML] add debug logging

* [ML] fix time range initialization

* [ML] fix with useMemo

* [ML] fix categorization validation check

* [ML] remove wrong setIsWizardReady

* [ML] revert page.tsx, update model_memory_estimator.ts and tests, skip failing tests

* [ML] adjust unit test description

* [ML] fix _runAdvancedValidation

* [ML] support async validation init of categorization job creator

* [ML] adjust unit tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Anomaly Detection ML anomaly detection :ml release_note:skip Skip the PR/issue when compiling release notes v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants