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.

Sign up for GitHub

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

Jump to bottom

[ML] Update allocations tooltip to clarify that it's per node #197099

Merged
merged 4 commits into from
Oct 23, 2024
Merged

[ML] Update allocations tooltip to clarify that it's per node #197099

merged 4 commits into from
Oct 23, 2024

Conversation

leemthompo
Copy link
Contributor

@leemthompo leemthompo commented Oct 21, 2024 •
edited by kibanamachine
Loading

Clarifies text to mention nodes, conditional on being not-serverless

h/t @simonhearne @jeffvestal

cc @serenachou

@leemthompo leemthompo added Team:ML Team label for ML (also use :ml) v8.16.0 backport:version Backport to applied version labels labels Oct 21, 2024
@leemthompo leemthompo self-assigned this Oct 21, 2024
@leemthompo leemthompo requested a review from a team as a code owner October 21, 2024 16:20
@leemthompo leemthompo added the release_note:skip Skip the PR/issue when compiling release notes label Oct 21, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@leemthompo
Copy link
Contributor Author

@jgowdyelastic could I ask for a review on this little change?

peteharverson
peteharverson reviewed Oct 23, 2024
View reviewed changes
x-pack/plugins/ml/public/application/memory_usage/nodes_overview/allocated_models.tsx Outdated
@@ -106,7 +106,8 @@ export const AllocatedModels: FC<AllocatedModelsProps> = ({
name: (
<EuiToolTip
content={i18n.translate('xpack.ml.trainedModels.nodesList.modelsList.allocationTooltip', {
defaultMessage: 'number_of_allocations times threads_per_allocation',
defaultMessage:
'Number of allocations per node multiplied by number of threads per allocation',
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently we show the same tooltip for stateful and serverless, and on serverless 'nodes' sort of don't exist. Although it is less clear, I'd therefore suggest

Number of allocations multiplied by number of threads per allocation

until we can add an extra check in for serverless (note there is some WIP here - #191960).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah well the request that sparked this change was to clarify whether it was indeed per node, so your edit doesn't actually change much 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we still return stuff about shards in basic es apis on serverless though currently

Copy link
Member

@jgowdyelastic jgowdyelastic Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This message should depend on whether we can show node information.
We have a showNodeInfo flag
const { showNodeInfo } = useEnabledFeatures();
It'll be false in serverless.

I'd suggest something like:

          content={
            showNodeInfo
              ? i18n.translate(
                  'xpack.ml.trainedModels.nodesList.modelsList.allocationTooltipNodes',
                  {
                    defaultMessage:
                      'Number of allocations per node multiplied by number of threads per allocation',
                  }
                )
              : i18n.translate('xpack.ml.trainedModels.nodesList.modelsList.allocationTooltip', {
                  defaultMessage:
                    'Number of allocations multiplied by number of threads per allocation',
                })
          }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Had a crack at implementing that in 8530fbd

@peteharverson peteharverson mentioned this pull request Oct 23, 2024
Closed
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
ml 4.5MB 4.5MB +244.0B

History

cc @leemthompo

jgowdyelastic
jgowdyelastic approved these changes Oct 23, 2024
View reviewed changes
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

@leemthompo leemthompo merged commit ea4ce57 into main Oct 23, 2024
25 checks passed
@leemthompo leemthompo deleted the leemthompo-patch-adams-tooltip branch October 23, 2024 15:30
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.16

https://github.com/elastic/kibana/actions/runs/11483208975

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 23, 2024
@leemthompo
[ML] Update allocations tooltip to clarify that it's per node (elasti…
cb4d4e9 
…c#197099)

Clarifies text to mention nodes, conditional on being not-serverless

(cherry picked from commit ea4ce57)
@kibanamachine kibanamachine mentioned this pull request Oct 23, 2024
Merged
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.16

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Oct 23, 2024
@kibanamachine @leemthompo
[8.16] [ML] Update allocations tooltip to clarify that it is per node (…
3d98f6c 
…#197099) (#197488)

# Backport

This will backport the following commits from `main` to `8.16`:
- [[ML] Update allocations tooltip to clarify that it&#x27;s per node
(#197099)](#197099)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Liam
Thompson","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-23T15:30:13Z","message":"[ML]
Update allocations tooltip to clarify that it's per node
(#197099)\n\nClarifies text to mention nodes, conditional on being
not-serverless","sha":"ea4ce57141019606b148016f667dd7a0cf98ff8c","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":[":ml","release_note:skip","v9.0.0","Team:ML","v8.16.0","backport:version"],"title":"[ML]
Update allocations tooltip to clarify that it's per
node","number":197099,"url":"https://github.com/elastic/kibana/pull/197099","mergeCommit":{"message":"[ML]
Update allocations tooltip to clarify that it's per node
(#197099)\n\nClarifies text to mention nodes, conditional on being
not-serverless","sha":"ea4ce57141019606b148016f667dd7a0cf98ff8c"}},"sourceBranch":"main","suggestedTargetBranches":["8.16"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/197099","number":197099,"mergeCommit":{"message":"[ML]
Update allocations tooltip to clarify that it's per node
(#197099)\n\nClarifies text to mention nodes, conditional on being
not-serverless","sha":"ea4ce57141019606b148016f667dd7a0cf98ff8c"}},{"branch":"8.16","label":"v8.16.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Liam Thompson <[email protected]>
@jgowdyelastic jgowdyelastic added v8.17.0 and removed backport:version Backport to applied version labels labels Oct 25, 2024
@jgowdyelastic jgowdyelastic added the backport:version Backport to applied version labels label Oct 25, 2024
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.16, 8.x

https://github.com/elastic/kibana/actions/runs/11520569348

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 25, 2024
@leemthompo
[ML] Update allocations tooltip to clarify that it's per node (elasti…
8e1a000 
…c#197099)

Clarifies text to mention nodes, conditional on being not-serverless

(cherry picked from commit ea4ce57)
@kibanamachine kibanamachine mentioned this pull request Oct 25, 2024
Merged
@kibanamachine
Copy link
Contributor

💔 Some backports could not be created

Status Branch Result
❌ 8.16 Cherrypick failed because the selected commit (ea4ce57) is empty. It looks like the commit was already backported in #197488
✅ 8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Manual backport

To create the backport manually run:

node scripts/backport --pr 197099

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Oct 25, 2024
@kibanamachine @leemthompo
[8.x] [ML] Update allocations tooltip to clarify that it&#x27;s per n…
1cf8b9d 
…ode (#197099) (#197846)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[ML] Update allocations tooltip to clarify that it&#x27;s per node
(#197099)](#197099)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Liam
Thompson","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-23T15:30:13Z","message":"[ML]
Update allocations tooltip to clarify that it's per node
(#197099)\n\nClarifies text to mention nodes, conditional on being
not-serverless","sha":"ea4ce57141019606b148016f667dd7a0cf98ff8c","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":[":ml","release_note:skip","v9.0.0","Team:ML","v8.16.0","backport:version","v8.17.0"],"title":"[ML]
Update allocations tooltip to clarify that it's per
node","number":197099,"url":"https://github.com/elastic/kibana/pull/197099","mergeCommit":{"message":"[ML]
Update allocations tooltip to clarify that it's per node
(#197099)\n\nClarifies text to mention nodes, conditional on being
not-serverless","sha":"ea4ce57141019606b148016f667dd7a0cf98ff8c"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/197099","number":197099,"mergeCommit":{"message":"[ML]
Update allocations tooltip to clarify that it's per node
(#197099)\n\nClarifies text to mention nodes, conditional on being
not-serverless","sha":"ea4ce57141019606b148016f667dd7a0cf98ff8c"}},{"branch":"8.16","label":"v8.16.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"url":"https://github.com/elastic/kibana/pull/197488","number":197488,"state":"MERGED","mergeCommit":{"sha":"3d98f6c3f7cdca73d184b8d5b6a103e7fcdfe779","message":"[8.16]
[ML] Update allocations tooltip to clarify that it is per node (#197099)
(#197488)\n\n# Backport\n\nThis will backport the following commits from
`main` to `8.16`:\n- [[ML] Update allocations tooltip to clarify that
it&#x27;s per
node\n(#197099)](https://github.com/elastic/kibana/pull/197099)\n\n<!---
Backport version: 9.4.3 -->\n\n### Questions ?\nPlease refer to the
[Backport
tool\ndocumentation](https://github.com/sqren/backport)\n\n<!--BACKPORT
[{\"author\":{\"name\":\"Liam\nThompson\",\"email\":\"[email protected]\"},\"sourceCommit\":{\"committedDate\":\"2024-10-23T15:30:13Z\",\"message\":\"[ML]\nUpdate
allocations tooltip to clarify that it's per
node\n(#197099)\\n\\nClarifies text to mention nodes, conditional on
being\nnot-serverless\",\"sha\":\"ea4ce57141019606b148016f667dd7a0cf98ff8c\",\"branchLabelMapping\":{\"^v9.0.0$\":\"main\",\"^v8.17.0$\":\"8.x\",\"^v(\\\\d+).(\\\\d+).\\\\d+$\":\"$1.$2\"}},\"sourcePullRequest\":{\"labels\":[\":ml\",\"release_note:skip\",\"v9.0.0\",\"Team:ML\",\"v8.16.0\",\"backport:version\"],\"title\":\"[ML]\nUpdate
allocations tooltip to clarify that it's
per\nnode\",\"number\":197099,\"url\":\"https://github.com/elastic/kibana/pull/197099\",\"mergeCommit\":{\"message\":\"[ML]\nUpdate
allocations tooltip to clarify that it's per
node\n(#197099)\\n\\nClarifies text to mention nodes, conditional on
being\nnot-serverless\",\"sha\":\"ea4ce57141019606b148016f667dd7a0cf98ff8c\"}},\"sourceBranch\":\"main\",\"suggestedTargetBranches\":[\"8.16\"],\"targetPullRequestStates\":[{\"branch\":\"main\",\"label\":\"v9.0.0\",\"branchLabelMappingKey\":\"^v9.0.0$\",\"isSourceBranch\":true,\"state\":\"MERGED\",\"url\":\"https://github.com/elastic/kibana/pull/197099\",\"number\":197099,\"mergeCommit\":{\"message\":\"[ML]\nUpdate
allocations tooltip to clarify that it's per
node\n(#197099)\\n\\nClarifies text to mention nodes, conditional on
being\nnot-serverless\",\"sha\":\"ea4ce57141019606b148016f667dd7a0cf98ff8c\"}},{\"branch\":\"8.16\",\"label\":\"v8.16.0\",\"branchLabelMappingKey\":\"^v(\\\\d+).(\\\\d+).\\\\d+$\",\"isSourceBranch\":false,\"state\":\"NOT_CREATED\"}]}]\nBACKPORT-->\n\nCo-authored-by:
Liam Thompson
<[email protected]>"}},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Liam Thompson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgowdyelastic jgowdyelastic jgowdyelastic approved these changes

@peteharverson peteharverson Awaiting requested review from peteharverson

Assignees

@leemthompo leemthompo

Labels
backport:version Backport to applied version labels :ml release_note:skip Skip the PR/issue when compiling release notes Team:ML Team label for ML (also use :ml) v8.16.0 v8.17.0 v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@leemthompo @elasticmachine @kibanamachine @peteharverson @jgowdyelastic