[ML] DF Analytics job creation: Add 'excludes' input field to form #53856

alvarezmelissa87 · 2019-12-30T21:45:49Z

Summary

Related meta issues:

Adds Excludes field to creation form that allows user to select fields or create custom patterns to exclude from analysis. All supported fields are included by default. Only supported field types for that type of job are available for selection in Excludes input (as unsupported fields will already be excluded).

Also updates supported types for dependent_variable for each type of job according to the docs: https://www.elastic.co/guide/en/elasticsearch/reference/master/ml-dfa-analysis-objects.html#regression-resources-standard

Adds error message for when a dependent_variable with more than 2 distinct values is selected for a classification job.

An initial iteration had both an Includes and an Excludes input but felt more prone to letting the user create an invalid job config since the excludes list has to specify which fields from the includes to exclude.

For example, two inputs allow you to create something like

"analyzed_fields" : {
        "includes" : [
          "Carrier",
          "dayOfWeek",
          "DestWeather",
          "Cancelled"
        ],
        "excludes" : [
          "AvgTicketPrice"
        ]
      },

which will break with an illegal_state_exception with reason: null when the the _start endpoint is hit.

When what you need is

"analyzed_fields" : {
        "includes" : [
          "dayOfWeek",
          "DestWeather",
          "Cancelled"
        ],
        "excludes" : [
          "dayOfWeek"
        ]
      }

That approach could work but it would all have to be handled on the front end as the returned error doesn't really give us a useful message to present to the user as to what the problem is.

It felt a lot more intuitive to just include all by default and only exclude undesired fields. The user can also work with the advanced editor for additional flexibility.

I'm happy to hear any feedback on this choice and update the way this works if anyone feels strongly about having both options in the form itself.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
~~- [ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

~~- [ ] This was checked for breaking API changes and was labeled appropriately~~
~~- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2019-12-30T21:45:52Z

Pinging @elastic/ml-ui (:ml)

kibanamachine · 2019-12-30T22:58:54Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 7828ea089dcd06ffb7d370adbfbdcb367f89a84c

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

walterra

LGTM, just added some minor suggestions. One more thought: As the code gets more complex with alle the different job types and options, it might be a good opportunity to make shouldAddFieldOption() and shouldAddAsDepVarOption() pure functions, move them out of the component into the main module scope and export them so we can create unit tests for them.