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] Transforms: Extend editing and creation options. #77370

Merged
merged 21 commits into from
Sep 21, 2020

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Sep 14, 2020

Summary

Fixes #67304, part of #67255.

Extends the available options in the edit transform flyout and the wizard details step.

Edit Flyout

Organizes the form to have basic form fields and expandable advanced sections.

image

Wizard Details Step

Adds an advanced expandable section.


Review note:

  • The edit form and wizard form use different types of approaches to manage form state. That's why for now some code (e.g. validators) is duplicated. The edit form uses the more up to date approach, the transform wizard was one of the first UIs we built out using React hooks and is a bit dated (it gets more fiddly as we add more stuff whereas the approach used for the edit form should allow use to grow the code base in a more structure way). I plan to consolidate some of the code and make use of the newer approach in the wizard in follow ups.
  • The structure of where form elements are in the expandable sections of the edit form and the wizard are not 100% in line, for example frequency is on the outer level for the edit form and in the advanced section of the wizard. Feedback on the structure is very welcome.

Checklist

For maintainers

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

)}
>
<EuiFieldText
placeholder="max_page_search_size"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be made the same placeholder text as in the edit form which is much more user-friendly.

image

If you delete the value in the wizard you get:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the placeholder in 01d852b. I will look into fixing the NaN issue in a follow up when consolidating the form code. Added a note to the meta issue here: #77604

Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

Thanks for adding a functional for the new feature! Just one suggestion.

@walterra
Copy link
Contributor Author

@peteharverson @pheyos pushed updates to your comments and PR is ready for another look 🤞

}

const defaultContinuousModeDelay = '60s';
const defaultTransformFrequency = '1m';
Copy link
Contributor

Choose a reason for hiding this comment

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

These settings don't seem to be copied over when cloning a job but reset to the defaults. Also even though it isn't exposed in the Create wizard, I think we should also copy over any docs_per_second setting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, added this to the issue about fixing other missing attributes when cloning transforms: #75264

Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

Functional tests 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.

Latest edits LGTM


const configValue = getNestedProperty(config, formStateAttribute.configFieldName, fallbackValue);

// only get depending values if we're not already in a call to get dependeing values.
Copy link
Member

@qn895 qn895 Sep 17, 2020

Choose a reason for hiding this comment

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

nit: should be depending

@qn895
Copy link
Member

qn895 commented Sep 17, 2020

LGTM 💯

@walterra
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
transform 503 +43 460

async chunks size

id value diff baseline
transform 661.1KB -107.5KB 768.6KB

History

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

@walterra walterra merged commit 4e7b7bf into elastic:master Sep 21, 2020
@walterra walterra deleted the ml-transforms-edit-3 branch September 21, 2020 14:55
walterra added a commit to walterra/kibana that referenced this pull request Sep 21, 2020
Extends the available options in the edit transform flyout and the wizard details step.
walterra added a commit that referenced this pull request Sep 21, 2020
Extends the available options in the edit transform flyout and the wizard details step.
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.

[ML] Transforms: Consider max_page_search_size in creation wizard
6 participants