Skip to content

Commit

Permalink
Lower minimum model memory limit value from 1MB to 1kB.
Browse files Browse the repository at this point in the history
  • Loading branch information
przemekwitek committed Nov 18, 2019
1 parent 43c154b commit 845924f
Show file tree
Hide file tree
Showing 8 changed files with 21 additions and 28 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1951,10 +1951,10 @@ public void testEstimateMemoryUsage() throws IOException {
.build());

// We are pretty liberal here as this test does not aim at verifying concrete numbers but rather end-to-end user workflow.
ByteSizeValue lowerBound = new ByteSizeValue(1, ByteSizeUnit.MB);
ByteSizeValue lowerBound = new ByteSizeValue(1, ByteSizeUnit.KB);
ByteSizeValue upperBound = new ByteSizeValue(1, ByteSizeUnit.GB);

// Data Frame has 10 rows, expect that the returned estimates fall within (1MB, 1GB) range.
// Data Frame has 10 rows, expect that the returned estimates fall within (1kB, 1GB) range.
EstimateMemoryUsageResponse response1 =
execute(
estimateMemoryUsageRequest, machineLearningClient::estimateMemoryUsage, machineLearningClient::estimateMemoryUsageAsync);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import java.util.Objects;

import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg;
import static org.elasticsearch.xpack.core.ml.dataframe.DataFrameAnalyticsConfig.MIN_MODEL_MEMORY_LIMIT;

public class EstimateMemoryUsageAction extends ActionType<EstimateMemoryUsageAction.Response> {

Expand Down Expand Up @@ -61,9 +60,8 @@ public static class Response extends ActionResponse implements ToXContentObject
private final ByteSizeValue expectedMemoryWithDisk;

public Response(@Nullable ByteSizeValue expectedMemoryWithoutDisk, @Nullable ByteSizeValue expectedMemoryWithDisk) {
this.expectedMemoryWithoutDisk =
expectedMemoryWithoutDisk != null ? max(expectedMemoryWithoutDisk, MIN_MODEL_MEMORY_LIMIT) : null;
this.expectedMemoryWithDisk = expectedMemoryWithDisk != null ? max(expectedMemoryWithDisk, MIN_MODEL_MEMORY_LIMIT) : null;
this.expectedMemoryWithoutDisk = expectedMemoryWithoutDisk;
this.expectedMemoryWithDisk = expectedMemoryWithDisk;
}

public Response(StreamInput in) throws IOException {
Expand Down Expand Up @@ -117,9 +115,5 @@ public boolean equals(Object other) {
public int hashCode() {
return Objects.hash(expectedMemoryWithoutDisk, expectedMemoryWithDisk);
}

private static <T extends Comparable<T>> T max(T value1, T value2) {
return value1.compareTo(value2) >= 0 ? value1 : value2;
}
}
}
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
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ public void testConstructor_NullValues() {

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

public void testConstructor() {
Expand Down
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."));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ setup:
body:
source: { index: "index-source" }
analysis: { outlier_detection: {} }
- match: { expected_memory_without_disk: "1mb" }
- match: { expected_memory_with_disk: "1mb" }
- match: { expected_memory_without_disk: "3kb" }
- match: { expected_memory_with_disk: "3kb" }

- do:
index:
Expand All @@ -65,8 +65,8 @@ setup:
body:
source: { index: "index-source" }
analysis: { outlier_detection: {} }
- match: { expected_memory_without_disk: "1mb" }
- match: { expected_memory_with_disk: "1mb" }
- match: { expected_memory_without_disk: "4kb" }
- match: { expected_memory_with_disk: "4kb" }

- do:
index:
Expand All @@ -80,5 +80,5 @@ setup:
body:
source: { index: "index-source" }
analysis: { outlier_detection: {} }
- match: { expected_memory_without_disk: "1mb" }
- match: { expected_memory_with_disk: "1mb" }
- match: { expected_memory_without_disk: "6kb" }
- match: { expected_memory_with_disk: "5kb" }

0 comments on commit 845924f

Please sign in to comment.