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] Data frame analytics: Advanced editor. #43989

Merged
merged 13 commits into from
Aug 28, 2019

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Aug 26, 2019

Summary

Adds an option to switch to an advanced (JSON) editor when creating an analytics job.

data-frame-analytics-advanced-editor-01

This builds upon the previous work for the modal for analytics job creation and the use of useReducer():

  • The files of the custom hook useCreateAnalyticsForm() have been further split up and there's now separate actions.ts and state.ts files.
  • To only allow updating what's really related to the form value's state via setFormState, the state structure has been updated and more fine grained actions have been added.
  • The user can enabled the advanced editor, but cannot move back to the original form (there's a help text in the UI about that).
  • The advanced editor component's (CreateAnalyticsAdvancedEditor) structure is based on the regular form, it still has an input field for the job ID and the toggle for optionally creating an index pattern. The fields for source/destination index are replaced by an editable JSON textarea input.
  • The advanced editor features mostly the same validation like the regular form. For example, if the source index isn't valid, an error will be shown in a CallOut below the editable area.

Checklist

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

For maintainers

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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.

A couple of minor comments on the use of id and one of the action labels.

I think it would be useful to expose the model_memory_limit as a setting in the editor. This should be given a reasonable (fixed) default for now - suggest 50MB. Then for 7.5 we should look at passing the config to the new endpoint to pre-populate this field with a sensible default - see #43740. Could an extra field also be added to the form editor for the model memory limit?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@walterra
Copy link
Contributor Author

@peteharverson In 1888b6e for both the form and advanced editor the default memory limit is set to 50mb.

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.

Latest edits LGTM

@alvarezmelissa87
Copy link
Contributor

I'm seeing some strange behavior when using the advanced editor

  • When I go to input the destination index, the value I'm inputting seems to get multiplied. Then if I backspace and type one letter the whole last value also suddenly appears.

analyticsAdvancedEditorTest

That behavior ^^ is accompanied by this error in the console:
image

  • I'm also seeing an issue where if I click outside of the advanced editor - the destination index value disappears

analyticsClickOutside

Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

Just left a comment on some odd behavior.

@walterra
Copy link
Contributor Author

@alvarezmelissa87 oh so sorry - looks like I missed saving one file change so it looked ok in my editor but git didn't pick it up :( - I just pushed the missing bit can you please try again?

@alvarezmelissa87
Copy link
Contributor

Tested again with latest updates and works as expected 🙌 Thanks, @walterra

Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

Latest changes LGTM ⚡️

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@walterra walterra merged commit b20c7fc into elastic:master Aug 28, 2019
@walterra walterra deleted the ml-dfa-advanced-editor branch August 28, 2019 07:05
walterra added a commit to walterra/kibana that referenced this pull request Aug 28, 2019
Adds an option to switch to an advanced (JSON) editor when creating an analytics job.

This builds upon the previous work for the modal for analytics job creation and the use of useReducer():

- The files of the custom hook useCreateAnalyticsForm() have been further split up and there's now separate actions.ts and state.ts files.
- To only allow updating what's really related to the form value's state via setFormState, the state structure has been updated and more fine grained actions have been added.
- The user can enabled the advanced editor, but cannot move back to the original form (there's a help text in the UI about that).
- The advanced editor component's (CreateAnalyticsAdvancedEditor) structure is based on the regular form, it still has an input field for the job ID and the toggle for optionally creating an index pattern. The fields for source/destination index are replaced by an editable JSON textarea input.
- The advanced editor features mostly the same validation like the regular form. For example, if the source index isn't valid, an error will be shown in a CallOut below the editable area.
walterra added a commit that referenced this pull request Aug 29, 2019
Adds an option to switch to an advanced (JSON) editor when creating an analytics job.

This builds upon the previous work for the modal for analytics job creation and the use of useReducer():

- The files of the custom hook useCreateAnalyticsForm() have been further split up and there's now separate actions.ts and state.ts files.
- To only allow updating what's really related to the form value's state via setFormState, the state structure has been updated and more fine grained actions have been added.
- The user can enabled the advanced editor, but cannot move back to the original form (there's a help text in the UI about that).
- The advanced editor component's (CreateAnalyticsAdvancedEditor) structure is based on the regular form, it still has an input field for the job ID and the toggle for optionally creating an index pattern. The fields for source/destination index are replaced by an editable JSON textarea input.
- The advanced editor features mostly the same validation like the regular form. For example, if the source index isn't valid, an error will be shown in a CallOut below the editable area.
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.

5 participants