Skip to content

Lower minimum model memory limit value from 1MB to 1kB. #49227

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

Merged
merged 5 commits into from
Nov 18, 2019
Merged

Lower minimum model memory limit value from 1MB to 1kB. #49227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d122296
Respect minimum model memory limit when returning memory usage estima…
przemekwitek Nov 18, 2019
7629b7f
Add unit test
przemekwitek Nov 18, 2019
7823abf
Fix MachineLearningIT test case
przemekwitek Nov 18, 2019
43c154b
Apply review comment
przemekwitek Nov 18, 2019
845924f
Lower minimum model memory limit value from 1MB to 1kB.
przemekwitek Nov 18, 2019
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
14 changes: 8 additions & 6 deletions client/rest-high-level/src/test/java/org/elasticsearch/client/MachineLearningIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,6 @@
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.greaterThan;
import static org.hamcrest.Matchers.greaterThanOrEqualTo;
import static org.hamcrest.Matchers.hasItem;
import static org.hamcrest.Matchers.hasItems;
Expand Down Expand Up @@ -1959,8 +1958,8 @@ public void testEstimateMemoryUsage() throws IOException {
EstimateMemoryUsageResponse response1 =
execute(
estimateMemoryUsageRequest, machineLearningClient::estimateMemoryUsage, machineLearningClient::estimateMemoryUsageAsync);
assertThat(response1.getExpectedMemoryWithoutDisk(), allOf(greaterThan(lowerBound), lessThan(upperBound)));
assertThat(response1.getExpectedMemoryWithDisk(), allOf(greaterThan(lowerBound), lessThan(upperBound)));
assertThat(response1.getExpectedMemoryWithoutDisk(), allOf(greaterThanOrEqualTo(lowerBound), lessThan(upperBound)));
assertThat(response1.getExpectedMemoryWithDisk(), allOf(greaterThanOrEqualTo(lowerBound), lessThan(upperBound)));

BulkRequest bulk2 = new BulkRequest()
.setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE);
Expand All @@ -1969,13 +1968,16 @@ public void testEstimateMemoryUsage() throws IOException {
}
highLevelClient().bulk(bulk2, RequestOptions.DEFAULT);

// Data Frame now has 100 rows, expect that the returned estimates will be greater than the previous ones.
// Data Frame now has 100 rows, expect that the returned estimates will be greater than or equal to the previous ones.
EstimateMemoryUsageResponse response2 =
execute(
estimateMemoryUsageRequest, machineLearningClient::estimateMemoryUsage, machineLearningClient::estimateMemoryUsageAsync);
assertThat(
response2.getExpectedMemoryWithoutDisk(), allOf(greaterThan(response1.getExpectedMemoryWithoutDisk()), lessThan(upperBound)));
assertThat(response2.getExpectedMemoryWithDisk(), allOf(greaterThan(response1.getExpectedMemoryWithDisk()), lessThan(upperBound)));
response2.getExpectedMemoryWithoutDisk(),
allOf(greaterThanOrEqualTo(response1.getExpectedMemoryWithoutDisk()), lessThan(upperBound)));
assertThat(
response2.getExpectedMemoryWithDisk(),
allOf(greaterThanOrEqualTo(response1.getExpectedMemoryWithDisk()), lessThan(upperBound)));
}

public void testPutFilter() throws Exception {
Expand Down
5 changes: 3 additions & 2 deletions ...ore/src/main/java/org/elasticsearch/xpack/core/ml/dataframe/DataFrameAnalyticsConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public class DataFrameAnalyticsConfig implements ToXContentObject, Writeable {
public static final String TYPE = "data_frame_analytics_config";

public static final ByteSizeValue DEFAULT_MODEL_MEMORY_LIMIT = new ByteSizeValue(1, ByteSizeUnit.GB);
public static final ByteSizeValue MIN_MODEL_MEMORY_LIMIT = new ByteSizeValue(1, ByteSizeUnit.MB);
public static final ByteSizeValue MIN_MODEL_MEMORY_LIMIT = new ByteSizeValue(1, ByteSizeUnit.KB);
/**
* This includes the overhead of thread stacks and data structures that the program might use that
* are not instrumented. But it does NOT include the memory used by loading the executable code.
Expand Down Expand Up @@ -442,7 +442,8 @@ private void applyMaxModelMemoryLimit() {
if (modelMemoryLimit.compareTo(MIN_MODEL_MEMORY_LIMIT) < 0) {
// Explicit setting lower than minimum is an error
throw ExceptionsHelper.badRequestException(
Messages.getMessage(Messages.JOB_CONFIG_MODEL_MEMORY_LIMIT_TOO_LOW, modelMemoryLimit));
Messages.getMessage(
Messages.JOB_CONFIG_MODEL_MEMORY_LIMIT_TOO_LOW, modelMemoryLimit, MIN_MODEL_MEMORY_LIMIT.getStringRep()));
}
if (maxModelMemoryIsSet && modelMemoryLimit.compareTo(maxModelMemoryLimit) > 0) {
// Explicit setting higher than limit is an error
Expand Down
2 changes: 1 addition & 1 deletion .../plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/AnalysisLimits.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ public AnalysisLimits(Long categorizationExamplesLimit) {

public AnalysisLimits(Long modelMemoryLimit, Long categorizationExamplesLimit) {
if (modelMemoryLimit != null && modelMemoryLimit < 1) {
String msg = Messages.getMessage(Messages.JOB_CONFIG_MODEL_MEMORY_LIMIT_TOO_LOW, modelMemoryLimit);
String msg = Messages.getMessage(Messages.JOB_CONFIG_MODEL_MEMORY_LIMIT_TOO_LOW, modelMemoryLimit, "1 MiB");
throw ExceptionsHelper.badRequestException(msg);
}
if (categorizationExamplesLimit != null && categorizationExamplesLimit < 0) {
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/messages/Messages.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ public final class Messages {
"Invalid detector rule: scope field ''{0}'' is invalid; select from {1}";
public static final String JOB_CONFIG_FIELDNAME_INCOMPATIBLE_FUNCTION = "field_name cannot be used with function ''{0}''";
public static final String JOB_CONFIG_FIELD_VALUE_TOO_LOW = "{0} cannot be less than {1,number}. Value = {2,number}";
public static final String JOB_CONFIG_MODEL_MEMORY_LIMIT_TOO_LOW = "model_memory_limit must be at least 1 MiB. Value = {0}";
public static final String JOB_CONFIG_MODEL_MEMORY_LIMIT_TOO_LOW = "model_memory_limit must be at least {1}. Value = {0}";
public static final String JOB_CONFIG_MODEL_MEMORY_LIMIT_GREATER_THAN_MAX =
"model_memory_limit [{0}] must be less than the value of the " +
MachineLearningField.MAX_MODEL_MEMORY_LIMIT.getKey() +
Expand Down
13 changes: 10 additions & 3 deletions ...t/java/org/elasticsearch/xpack/core/ml/action/EstimateMemoryUsageActionResponseTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package org.elasticsearch.xpack.core.ml.action;

import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.common.unit.ByteSizeUnit;
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.test.AbstractSerializingTestCase;
Expand Down Expand Up @@ -39,9 +40,15 @@ public void testConstructor_NullValues() {
assertThat(response.getExpectedMemoryWithDisk(), nullValue());
}

public void testConstructor_SmallValues() {
Response response = new Response(new ByteSizeValue(120, ByteSizeUnit.KB), new ByteSizeValue(30, ByteSizeUnit.KB));
assertThat(response.getExpectedMemoryWithoutDisk(), equalTo(new ByteSizeValue(120, ByteSizeUnit.KB)));
assertThat(response.getExpectedMemoryWithDisk(), equalTo(new ByteSizeValue(30, ByteSizeUnit.KB)));
}

public void testConstructor() {
Response response = new Response(new ByteSizeValue(2048), new ByteSizeValue(1024));
assertThat(response.getExpectedMemoryWithoutDisk(), equalTo(new ByteSizeValue(2048)));
assertThat(response.getExpectedMemoryWithDisk(), equalTo(new ByteSizeValue(1024)));
Response response = new Response(new ByteSizeValue(20, ByteSizeUnit.MB), new ByteSizeValue(10, ByteSizeUnit.MB));
assertThat(response.getExpectedMemoryWithoutDisk(), equalTo(new ByteSizeValue(20, ByteSizeUnit.MB)));
assertThat(response.getExpectedMemoryWithDisk(), equalTo(new ByteSizeValue(10, ByteSizeUnit.MB)));
}
}
10 changes: 4 additions & 6 deletions ...rc/test/java/org/elasticsearch/xpack/core/ml/dataframe/DataFrameAnalyticsConfigTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,18 +232,16 @@ public void testInvalidModelMemoryLimits() {
DataFrameAnalyticsConfig.Builder builder = new DataFrameAnalyticsConfig.Builder();

// All these are different ways of specifying a limit that is lower than the minimum
assertTooSmall(expectThrows(ElasticsearchStatusException.class,
() -> builder.setModelMemoryLimit(new ByteSizeValue(1048575, ByteSizeUnit.BYTES)).build()));
assertTooSmall(expectThrows(ElasticsearchStatusException.class,
() -> builder.setModelMemoryLimit(new ByteSizeValue(0, ByteSizeUnit.BYTES)).build()));
assertTooSmall(expectThrows(ElasticsearchStatusException.class,
() -> builder.setModelMemoryLimit(new ByteSizeValue(-1, ByteSizeUnit.BYTES)).build()));
assertTooSmall(expectThrows(ElasticsearchStatusException.class,
() -> builder.setModelMemoryLimit(new ByteSizeValue(1023, ByteSizeUnit.KB)).build()));
() -> builder.setModelMemoryLimit(new ByteSizeValue(0, ByteSizeUnit.BYTES)).build()));
assertTooSmall(expectThrows(ElasticsearchStatusException.class,
() -> builder.setModelMemoryLimit(new ByteSizeValue(0, ByteSizeUnit.KB)).build()));
assertTooSmall(expectThrows(ElasticsearchStatusException.class,
() -> builder.setModelMemoryLimit(new ByteSizeValue(0, ByteSizeUnit.MB)).build()));
assertTooSmall(expectThrows(ElasticsearchStatusException.class,
() -> builder.setModelMemoryLimit(new ByteSizeValue(1023, ByteSizeUnit.BYTES)).build()));
}

public void testNoMemoryCapping() {
Expand Down Expand Up @@ -342,6 +340,6 @@ public void testPreventVersionInjection() throws IOException {
}

private static void assertTooSmall(ElasticsearchStatusException e) {
assertThat(e.getMessage(), startsWith("model_memory_limit must be at least 1 MiB."));
assertThat(e.getMessage(), startsWith("model_memory_limit must be at least 1kb."));
}
}
15 changes: 12 additions & 3 deletions ...rg/elasticsearch/xpack/ml/dataframe/process/results/MemoryUsageEstimationResultTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
package org.elasticsearch.xpack.ml.dataframe.process.results;

import org.elasticsearch.common.unit.ByteSizeUnit;
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.test.AbstractXContentTestCase;
Expand Down Expand Up @@ -43,9 +44,17 @@ public void testConstructor_NullValues() {
assertThat(result.getExpectedMemoryWithDisk(), nullValue());
}

public void testConstructor_SmallValues() {
MemoryUsageEstimationResult result =
new MemoryUsageEstimationResult(new ByteSizeValue(120, ByteSizeUnit.KB), new ByteSizeValue(30, ByteSizeUnit.KB));
assertThat(result.getExpectedMemoryWithoutDisk(), equalTo(new ByteSizeValue(120, ByteSizeUnit.KB)));
assertThat(result.getExpectedMemoryWithDisk(), equalTo(new ByteSizeValue(30, ByteSizeUnit.KB)));
}

public void testConstructor() {
MemoryUsageEstimationResult result = new MemoryUsageEstimationResult(new ByteSizeValue(2048), new ByteSizeValue(1024));
assertThat(result.getExpectedMemoryWithoutDisk(), equalTo(new ByteSizeValue(2048)));
assertThat(result.getExpectedMemoryWithDisk(), equalTo(new ByteSizeValue(1024)));
MemoryUsageEstimationResult result =
new MemoryUsageEstimationResult(new ByteSizeValue(20, ByteSizeUnit.MB), new ByteSizeValue(10, ByteSizeUnit.MB));
assertThat(result.getExpectedMemoryWithoutDisk(), equalTo(new ByteSizeValue(20, ByteSizeUnit.MB)));
assertThat(result.getExpectedMemoryWithDisk(), equalTo(new ByteSizeValue(10, ByteSizeUnit.MB)));
}
}