Lower minimum model memory limit value from 1MB to 1kB.#49227
Lower minimum model memory limit value from 1MB to 1kB.#49227przemekwitek merged 5 commits intoelastic:masterfrom
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
droberts195
left a comment
There was a problem hiding this comment.
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 |
droberts195
left a comment
There was a problem hiding this comment.
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
1MBto1kB.Relates #49168