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] Use perAllocation and perDeployment memory usage in the model assignment planner #98874

Merged
merged 42 commits into from Nov 6, 2023

Conversation

valeriy42
Copy link
Contributor

@valeriy42 valeriy42 commented Aug 25, 2023

Building upon #98139, this PR extends the model assignment planning algorithms and the linear solver to use the extended memory fields. It also adds unit tests to verify the new behavior.

I needed to adjust the old unit tests since we use the estimateMemoryUsage routine, which would compute 2*memoryBytes + 240 MB as the memory requirement. Previously, in the unit tests, we were simply using memoryBytes field value.

@valeriy42 valeriy42 added >non-issue :ml Machine learning labels Aug 25, 2023
@mattc58 mattc58 added v8.12.0 and removed v8.11.0 labels Oct 4, 2023
@valeriy42 valeriy42 changed the title [ML] WIP Assignment Planner change [ML] Assignment Planner change Oct 9, 2023
@valeriy42 valeriy42 changed the title [ML] Assignment Planner change [ML] Use perAllocation and perDeployment memory usage in the model assignment planner Oct 9, 2023
@valeriy42 valeriy42 marked this pull request as ready for review October 9, 2023 09:13
@elasticsearchmachine elasticsearchmachine added the Team:ML Meta label for the ML team label Oct 9, 2023
@elasticsearchmachine
Copy link
Collaborator

Hi @valeriy42, I've created a changelog YAML for you.

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/ml-core (Team:ML)

Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

LGTM

@droberts195
Copy link
Contributor

@valeriy42 this is failing CI because of this:

Forbidden method invocation: java.lang.String#format(java.lang.String,java.lang.Object[]) [Uses default locale]
in org.elasticsearch.upgrades.MlAssignmentPlannerUpgradeIT (MlAssignmentPlannerUpgradeIT.java:141)

You can fix that by using Strings.format instead of String.format. (Where Strings is an Elastic class that wraps up adding the locale.)

@valeriy42
Copy link
Contributor Author

@elasticmachine update branch

Copy link
Contributor

@tveasey tveasey left a comment

Choose a reason for hiding this comment

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

Good work on this! LGTM

@valeriy42 valeriy42 merged commit aa2f6e7 into elastic:main Nov 6, 2023
13 checks passed
valeriy42 added a commit to elastic/eland that referenced this pull request Nov 6, 2023
This PR adds an ability to estimate per deployment and per allocation memory usage of NLP transformer models. It uses torch.profiler and performs logs the peak memory usage during the inference.

This information is then used in Elasticsearch to provision models with sufficient memory (elastic/elasticsearch#98874).
@valeriy42 valeriy42 deleted the update-assignment-planner branch November 6, 2023 11:18
@valeriy42 valeriy42 restored the update-assignment-planner branch November 6, 2023 14:33
valeriy42 added a commit that referenced this pull request Nov 6, 2023
valeriy42 added a commit that referenced this pull request Nov 6, 2023
…model assignment planner (#98874)" (#101834)

There were a number of BWC test failures after the PR was merged today. I'll revert it and investigate the failures locally.

Reverts #98874
valeriy42 added a commit to valeriy42/elasticsearch that referenced this pull request Nov 7, 2023
…signment planner (elastic#98874)

Building upon elastic#98139, this PR extends the model assignment planning algorithms and the linear solver to use the extended memory fields. It also adds unit tests to verify the new behavior.

I needed to adjust the old unit tests since we use the estimateMemoryUsage routine, which would compute 2*memoryBytes + 240 MB as the memory requirement. Previously, in the unit tests, we were simply using memoryBytes field value.
valeriy42 added a commit that referenced this pull request Nov 7, 2023
…in the model assignment planner" (#101853)

The original PR #98874 missed the memory overhead adjustment from #86416. As it caused some BWC test failures on the CI, I reverted it in #101834.

This PR reintegrates the functionality and extends the BWC integration test with the memory constant depending on the version of the old cluster.
droberts195 added a commit to droberts195/elasticsearch that referenced this pull request Dec 11, 2023
…model assignment planner"

This reverts commit 31ca2f7.

The functionality of elastic#98874 is being removed from 8.12 because it
means that models which were working successfully on 2GB nodes in
8.11 will no longer fit on 2GB nodes. This will be frustrating for
trial users.

Before 8.13 we need to do a more thorough assessment of which
models will and won't fit on 2GB nodes as a result of better
memory estimation. It may be possible to tweak the memory usage
estimation so that we require more memory than 8.11 but not so
much more that our recommended trial models no longer fit onto
2GB nodes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :ml Machine learning Team:ML Meta label for the ML team v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants