Skip to content

[DOCS] Move anomaly detection job resource definitions into APIs #49700

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

Merged
merged 16 commits into from
Dec 6, 2019

Conversation

lcawl
Copy link
Contributor

@lcawl lcawl commented Nov 29, 2019

This PR adds tagged sections that contain descriptions of the anomaly detection job resource properties in https://github.com/elastic/elasticsearch/blob/master/docs/reference/ml/ml-shared.asciidoc. It removes the job resource definitions page (https://www.elastic.co/guide/en/elasticsearch/reference/master/ml-job-resource.html) and uses the shared tagged sections to put that information directly into the appropriate APIs.

NOTE: As a result of these changes, users no longer need to flip between the job resource page and the API page to find all the necessary details. However, from a maintenance stand-point, it means that when parameters or properties are added or removed, each of the affected APIs' pages must be updated appropriately.

Subsequent PRs will be opened to (1) add missing indicators of which properties are required or optional in the create job API, (2) minimize some of the overly long property descriptions, and (3) merge some of the varied definitions for "job_id", since it seems unlikely so many variations are needed.

Preview:
http://elasticsearch_49700.docs-preview.app.elstc.co/guide/en/elasticsearch/reference/master/ml-put-job.html
http://elasticsearch_49700.docs-preview.app.elstc.co/guide/en/elasticsearch/reference/master/ml-get-job.html
http://elasticsearch_49700.docs-preview.app.elstc.co/guide/en/elasticsearch/reference/master/ml-update-job.html
...

@lcawl lcawl added WIP :ml Machine learning v8.0.0 labels Nov 29, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (:ml)

Copy link
Contributor

@edsavage edsavage left a comment

Choose a reason for hiding this comment

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

Looks like the CI build failed due to...

WARNING: ml/anomaly-detection/apis/put-job.asciidoc: line 36: tag 'job-id-def' not found in include file: /tmp/docsbuild/cyKWLdPar0/elasticsearch/docs/reference/rest-api/common-parms.asciidoc

@lcawl lcawl force-pushed the ml-common-parms branch 2 times, most recently from a74af8b to 6c988de Compare December 3, 2019 01:35
@edsavage
Copy link
Contributor

edsavage commented Dec 4, 2019

CI failed due to

Invalid json in reference/ml/anomaly-detection/apis/get-job.asciidoc154:207// TESTRESPONSE. The error is:
23:18:50 expecting '}' or ',' but got current char '.' with an int value of 46

This is due to some dodgy JSON introduced in commit 9c997cf

@lcawl
Copy link
Contributor Author

lcawl commented Dec 5, 2019

@elasticmachine elasticseach-ci/docs-check

@lcawl
Copy link
Contributor Author

lcawl commented Dec 5, 2019

@elasticmachine run elasticsearch-ci/docs-check

@lcawl lcawl changed the title [DOCS] Drafts ML common-parms [DOCS] Move anomaly detection job resource definitions into APIs Dec 5, 2019
@lcawl lcawl removed the WIP label Dec 5, 2019
@lcawl lcawl marked this pull request as ready for review December 5, 2019 22:23
@lcawl lcawl requested review from szabosteve and sophiec20 December 5, 2019 22:24
@lcawl lcawl added the >docs General docs changes label Dec 5, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-docs (>docs)

Copy link
Contributor

@szabosteve szabosteve left a comment

Choose a reason for hiding this comment

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

LGTM!
I left a few comments and suggestions, nothing blocker though. Thank you for this enormous work!

include::{docdir}/ml/ml-shared.asciidoc[tag=influencers]

`latency`:::
(time units)
Copy link
Contributor

@szabosteve szabosteve Dec 6, 2019

Choose a reason for hiding this comment

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

Suggested change
(time units)
(<<time-units,time units>>)

tag::dependent_variable[]
`dependent_variable`::
(Required, string) Defines which field of the document is to be predicted.
Defines which field of the document is to be predicted.
Copy link
Contributor

@szabosteve szabosteve Dec 6, 2019

Choose a reason for hiding this comment

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

I'm wondering if we can put it back until I start to work on the DFA related parameters? I'd like to be consistent, too, but then we need to delete the name and the indicators between parentheses in the cases of dependent_variable, gamma, eta, lambda, feature_bag_fraction, maximum_number_trees, and so on, then open a PR that adds the removed info back to the API docs. Finally, merge and backport the two PRs (this one and the one that put the titles and indicators back to the API docs) in sync.
The other option is to leave it as-is for now and I'll remove the titles in this file and add them back in the API docs in one PR later. WDYT? If my train of thought is obscure and I explained my concern poorly, please let me know.

Suggested change
Defines which field of the document is to be predicted.
`dependent_variable`::
(Required, string) Defines which field of the document is to be predicted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, there was no reason for me to change this, since it isn't used by any of the APIs I updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>docs General docs changes :ml Machine learning v7.5.2 v7.6.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants