[ML] Enabling mml estimation in data recognizer module setup #64900
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jgowdyelastic
requested review from
darnautov,
peteharverson,
qn895 and
alvarezmelissa87
jgowdyelastic
added
:ml
Feature:Anomaly Detection
|
Pinging @elastic/ml-ui (:ml) |
5 tasks
darnautov
reviewed
Apr 30, 2020
x-pack/plugins/ml/server/models/data_recognizer/data_recognizer.ts
Outdated
Show resolved
Hide resolved
|
Code LGTM, but I don't see unit or functional tests 👀 It'd nice to have a basic unit test for |
|
@darnautov functional tests added in de1f4a4 |
|
@pheyos added you as a reviewer of the tests. |
darnautov
reviewed
May 1, 2020
| prefix: 'pf2_', | ||
| indexPatternName: 'kibana_sample_data_logs', | ||
| startDatafeed: false, | ||
| estimateModelMemory: true, |
There was a problem hiding this comment.
probably worth to omit this parameter to validate that estimateModelMemory is enabled by default
darnautov
reviewed
May 1, 2020
Comment on lines
+254
to
+257
| }: { | ||
| body: { | ||
| jobs: Job[]; | ||
| }; |
There was a problem hiding this comment.
would be better to provide a return type for getAnomalyDetectionJob method
There was a problem hiding this comment.
I agree, but would touch other files which use it, so should be done in a refactor follow up.
|
@elasticmachine merge upstream |
…github.com:jgowdyelastic/kibana into enabling-mml-estimation-in-data-recognizer-modules
|
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
shahzad31
approved these changes
May 4, 2020
jgowdyelastic
deleted the
enabling-mml-estimation-in-data-recognizer-modules
branch
jgowdyelastic
added a commit
to jgowdyelastic/kibana
that referenced
this pull request
May 4, 2020
…#64900) * [ML] Enabling mml estimation in data recognizer module setup * small refactor * adding functional tests * increasing uptime test timeout * tiny refactor * checking for default setting * testng flakey uptime test * catching erros in mml estimation * lowering timeout * ensuing data is present for ML tests * adding await Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
jgowdyelastic
added a commit
that referenced
this pull request
May 4, 2020
…#65141) * [ML] Enabling mml estimation in data recognizer module setup * small refactor * adding functional tests * increasing uptime test timeout * tiny refactor * checking for default setting * testng flakey uptime test * catching erros in mml estimation * lowering timeout * ensuing data is present for ML tests * adding await Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Enables the model memory limit estimation by default for the
/api/ml/modules/setupendpoint.Also fixes an issue where the module's
querywas being used for retrieving the index time range and for the estimation.The module's
queryis used for the "recognising" aspect of this feature and is not appropriate for mml estimation.Instead the queries from each datafeed are used instead. Unless there is a common time field where a general
match_allis used.Part of #48510
Also fixes issue with uptime ML integration functional tests where the index data hadn't been loaded before the tests start.
Unit or functional tests were updated or added to match the most common scenarios
This was checked for breaking API changes and was labeled appropriately