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

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

Merged
merged 4 commits into from
Oct 23, 2024
+19 −6
Merged

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

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cb817cf
Update allocations tooltip to clarify that it's per node
leemthompo Oct 21, 2024
16855b1
Tweak
leemthompo Oct 22, 2024
8530fbd
Condition tooltip based on deployment type
leemthompo Oct 23, 2024
5f97e64
Update translations
leemthompo Oct 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion x-pack/plugins/ml/public/application/memory_usage/nodes_overview/allocated_models.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

})}
>
<span>
Expand Down