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] Always enable next button in advanced job validation #78931

Conversation

jgowdyelastic
Copy link
Member

@jgowdyelastic jgowdyelastic commented Sep 30, 2020

When validating an advanced job, if the validation results contain errors, the user should still be able to progress to the next step to create the job.
This will allow jobs t be created for indices which do not have any data, and so can fail field cardinality checks.

image

Relates to #78784

Checklist

Delete any items that are not applicable to this PR.

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

LGTM - Just a question on the structure: Since the code in validation.tsx already does the same check if the job type is advanced, could that also include the check if it's valid so that the callout component could be more "dumb" and don't do any checks by itself?

@jgowdyelastic
Copy link
Member Author

LGTM - Just a question on the structure: Since the code in validation.tsx already does the same check if the job type is advanced, could that also include the check if it's valid so that the callout component could be more "dumb" and don't do any checks by itself?

I originally thought that the callout component should be written to be a little bit smart, so it would be reusable.
But thinking about it now, that is very unlikely, so i'll make this suggested change.

@sophiec20
Copy link
Contributor

Should we be specifically handling the case where zero data points are found? Presumably this could be a warning validation.

Can we avoid the misleading message field blah is not an aggregatable field - unless it actually is not aggregatable, rather than not there.

@jgowdyelastic
Copy link
Member Author

jgowdyelastic commented Sep 30, 2020

Should we be specifically handling the case where zero data points are found? Presumably this could be a warning validation.

Yes, we will need to enhance the job validation to check for this. Currently there is a limitation in the way the field cardinality check works in that any error thrown is reported as a field blah is not an aggregatable field failure.
This check should be smarter.

This PR has been added to "unblock" a user who runs into this limitation and to address the first checkbox in the original issue #78784
Further work will be needed to fix the check and improve the validation failure messaging, covered by the second checkbox in #78784

Alternatively we could park this PR and work on the second part. Which would hopefully mean this change wouldn't be needed. @peteharverson ?

Thinking about it more, I'm leaning towards abandoning this PR in favour of the larger change to the validation.

@peteharverson
Copy link
Contributor

I agree with @jgowdyelastic that we should park this PR, and look at improving the validation to provide better messaging when there is no data, as well as allowing the user to progress through the advanced wizard even if validation fails.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id value diff baseline
ml 1186 +1 1185

async chunks size

id value diff baseline
ml 10.5MB +2.5KB 10.5MB

History

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

@jgowdyelastic jgowdyelastic deleted the always-enable-next-for-advanced-job-validation branch October 3, 2020 08:24
@walterra
Copy link
Contributor

This will be fixed in #86114.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants