-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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
Lower minimum model memory limit value from 1MB to 1kB. #49227
Lower minimum model memory limit value from 1MB to 1kB. #49227
Conversation
…tion from _estimate endpoint.
Pinging @elastic/ml-core (:ml) |
caada70
to
7629b7f
Compare
...gin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/EstimateMemoryUsageAction.java
Outdated
Show resolved
Hide resolved
...gin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/EstimateMemoryUsageAction.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given what the C++ does - see https://github.com/elastic/ml-cpp/pull/584/files#diff-8c1b056d827cb41f2dbd4a97a5e202deR59 - it might be better to change DataFrameAnalyticsConfig.MIN_MODEL_MEMORY_LIMIT
to 1kb
instead of messing with the estimates in the "estimate" action. If we cap at 1mb
here then there is no point in the C++ outputting values that are less than a kilobyte.
I am pretty sure we once discussed whether model memory limits for data frame analytics jobs should be whole numbers of megabytes like they are for anomaly detection jobs. I cannot find the whole discussion now. It was touched on in elastic/ml-cpp#584 (comment), but I think there was a bigger discussion somewhere where there was a desire to allow model memory limits less than a megabyte for really small jobs.
Changing the min model memory limit to 1kb
might have side other effects though, so please check how it's used throughout the code.
I didn't want to change C++ as it's not the layer that has the knowledge of this minimum memory limit.
I'm all for lowering the limit to
The minimum limit of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM, but please can you change the PR title and description before merging
822be32
to
845924f
Compare
Done. |
This PR lowers the minimum model memory limit from
1MB
to1kB
.Relates #49168