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

Add config for request timeout in seconds #1019

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ private void initModelStore() {
null,
1,
100,
configManager.getDefaultResponseTimeout(),
configManager.getDefaultResponseTimeoutSeconds(),
defaultModelName,
configManager.getPreloadModel());
modelManager.updateModel(archive.getModelName(), workers, workers);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -188,13 +188,13 @@ private void handleRegisterModel(
int maxBatchDelay = registerModelRequest.getMaxBatchDelay();
int initialWorkers = registerModelRequest.getInitialWorkers();
boolean synchronous = registerModelRequest.isSynchronous();
int responseTimeout = registerModelRequest.getResponseTimeout();
int responseTimeoutSeconds = registerModelRequest.getResponseTimeoutSeconds();
String preloadModel = registerModelRequest.getPreloadModel();
if (preloadModel == null) {
preloadModel = ConfigManager.getInstance().getPreloadModel();
}
if (responseTimeout == -1) {
responseTimeout = ConfigManager.getInstance().getDefaultResponseTimeout();
if (responseTimeoutSeconds == -1) {
responseTimeoutSeconds = ConfigManager.getInstance().getDefaultResponseTimeoutSeconds();
}
Manifest.RuntimeType runtimeType = null;
if (runtime != null) {
Expand All @@ -217,7 +217,7 @@ private void handleRegisterModel(
handler,
batchSize,
maxBatchDelay,
responseTimeout,
responseTimeoutSeconds,
null,
preloadModel);
} catch (IOException | InterruptedException | ExecutionException | TimeoutException e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public class RegisterModelRequest {
private boolean synchronous;

@SerializedName("response_timeout")
private int responseTimeout;
private int responseTimeoutSeconds;

@SerializedName("url")
private String modelUrl;
Expand All @@ -61,7 +61,15 @@ public RegisterModelRequest(QueryStringDecoder decoder) {
"initial_workers",
ConfigManager.getInstance().getConfiguredDefaultWorkersPerModel());
synchronous = Boolean.parseBoolean(NettyUtils.getParameter(decoder, "synchronous", "true"));
responseTimeout = NettyUtils.getIntParameter(decoder, "response_timeout", -1);

// TODO Fix this so it matches the documentation, where timeouts are specified in seconds.
// For now, we're being extra careful about backwards compatibility.
// So, assume parameter is in minutes, and convert to seconds internally.
responseTimeoutSeconds = 60 * NettyUtils.getIntParameter(decoder, "response_timeout", -1);
if (responseTimeoutSeconds < 0) {
responseTimeoutSeconds = -1;
}

modelUrl = NettyUtils.getParameter(decoder, "url", null);
preloadModel = NettyUtils.getParameter(decoder, "preload_model", null);
}
Expand All @@ -71,7 +79,7 @@ public RegisterModelRequest() {
maxBatchDelay = 100;
synchronous = true;
initialWorkers = ConfigManager.getInstance().getConfiguredDefaultWorkersPerModel();
responseTimeout = -1;
responseTimeoutSeconds = -1;
preloadModel = null;
}

Expand Down Expand Up @@ -103,8 +111,8 @@ public Boolean isSynchronous() {
return synchronous;
}

public Integer getResponseTimeout() {
return responseTimeout;
public Integer getResponseTimeoutSeconds() {
return responseTimeoutSeconds;
}

public String getModelUrl() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,11 @@ public final class ConfigManager {
private static final String MMS_LOAD_MODELS = "load_models";
private static final String MMS_BLACKLIST_ENV_VARS = "blacklist_env_vars";
private static final String MMS_DEFAULT_WORKERS_PER_MODEL = "default_workers_per_model";

private static final String MMS_DEFAULT_RESPONSE_TIMEOUT = "default_response_timeout";
private static final String MMS_DEFAULT_RESPONSE_TIMEOUT_SECONDS =
"default_response_timeout_seconds";

private static final String MMS_UNREGISTER_MODEL_TIMEOUT = "unregister_model_timeout";
private static final String MMS_NUMBER_OF_NETTY_THREADS = "number_of_netty_threads";
private static final String MMS_NETTY_CLIENT_THREADS = "netty_client_threads";
Expand Down Expand Up @@ -519,8 +523,20 @@ private int getIntProperty(String key, int def) {
return Integer.parseInt(value);
}

public int getDefaultResponseTimeout() {
return Integer.parseInt(prop.getProperty(MMS_DEFAULT_RESPONSE_TIMEOUT, "120"));
public int getDefaultResponseTimeoutSeconds() {
// TODO The MMS_DEFAULT_RESPONSE_TIMEOUT variable was never intended to represent minutes,
// but due to a bug that's what it did. We'd like to remove this and match the documented
// behavior, but for now we're being cautious about backward compatibility.

// Check both properties, prefer seconds if provided, convert to seconds for return value
int timeoutSeconds =
Integer.parseInt(prop.getProperty(MMS_DEFAULT_RESPONSE_TIMEOUT_SECONDS, "-1"));
if (timeoutSeconds < 0) {
int timeoutMinutes =
Integer.parseInt(prop.getProperty(MMS_DEFAULT_RESPONSE_TIMEOUT, "120"));
timeoutSeconds = 60 * timeoutMinutes;
}
return timeoutSeconds;
}

public int getUnregisterModelTimeout() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public class Model {
private String preloadModel;
private AtomicInteger port; // Port on which the model server is running
private ReentrantLock lock;
private int responseTimeout;
private int responseTimeoutSeconds;
private WorkerThread serverThread;
// Total number of subsequent inference request failures
private AtomicInteger failedInfReqs;
Expand Down Expand Up @@ -198,12 +198,12 @@ public void resetFailedInfReqs() {
failedInfReqs.set(0);
}

public int getResponseTimeout() {
return ConfigManager.getInstance().isDebug() ? Integer.MAX_VALUE : responseTimeout;
public int getResponseTimeoutSeconds() {
return ConfigManager.getInstance().isDebug() ? Integer.MAX_VALUE : responseTimeoutSeconds;
}

public void setResponseTimeout(int responseTimeout) {
this.responseTimeout = responseTimeout;
public void setResponseTimeoutSeconds(int responseTimeoutSeconds) {
this.responseTimeoutSeconds = responseTimeoutSeconds;
}

public WorkerThread getServerThread() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ public ModelArchive registerModel(String url, String defaultModelName, String pr
null,
1,
100,
configManager.getDefaultResponseTimeout(),
configManager.getDefaultResponseTimeoutSeconds(),
defaultModelName,
preloadModel);
}
Expand All @@ -90,7 +90,7 @@ public ModelArchive registerModel(
String handler,
int batchSize,
int maxBatchDelay,
int responseTimeout,
int responseTimeoutSeconds,
String defaultModelName,
String preloadModel)
throws ModelException, IOException, InterruptedException, ExecutionException,
Expand Down Expand Up @@ -121,7 +121,7 @@ public ModelArchive registerModel(
Model model = new Model(archive, configManager.getJobQueueSize(), preloadModel);
model.setBatchSize(batchSize);
model.setMaxBatchDelay(maxBatchDelay);
model.setResponseTimeout(responseTimeout);
model.setResponseTimeoutSeconds(responseTimeoutSeconds);
Model existingModel = models.putIfAbsent(modelName, model);
if (existingModel != null) {
// model already exists
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,13 +139,13 @@ public WorkerThread(

private void runWorker()
throws WorkerInitializationException, InterruptedException, FileNotFoundException {
int responseTimeout = model.getResponseTimeout();
int responseTimeoutSeconds = model.getResponseTimeoutSeconds();
while (isRunning()) {
req = aggregator.getRequest(backendChannel.id().asLongText(), state);
backendChannel.writeAndFlush(req).sync();
long begin = System.currentTimeMillis();
// TODO: Change this to configurable param
ModelWorkerResponse reply = replies.poll(responseTimeout, TimeUnit.MINUTES);
ModelWorkerResponse reply = replies.poll(responseTimeoutSeconds, TimeUnit.SECONDS);
long duration = System.currentTimeMillis() - begin;
logger.info("Backend response time: {}", duration);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ public void test()
ConfigManager configManager = ConfigManager.getInstance();
configManager.setProperty("keystore", "src/test/resources/keystore.p12");
Assert.assertEquals("true", configManager.getEnableEnvVarsConfig());
Assert.assertEquals(130, configManager.getDefaultResponseTimeout());
Assert.assertEquals(60 * 130, configManager.getDefaultResponseTimeoutSeconds());

Dimension dimension;
List<Metric> metrics = new ArrayList<>();
Expand Down Expand Up @@ -124,6 +124,20 @@ public void testNoEnvVars()
ConfigManager.init(args);
ConfigManager configManager = ConfigManager.getInstance();
Assert.assertEquals("false", configManager.getEnableEnvVarsConfig());
Assert.assertEquals(120, configManager.getDefaultResponseTimeout());
Assert.assertEquals(60 * 120, configManager.getDefaultResponseTimeoutSeconds());
}

@Test
public void testResponseTimeoutSeconds()
throws IOException, GeneralSecurityException, IllegalAccessException,
NoSuchFieldException, ClassNotFoundException {
System.setProperty("mmsConfigFile", "src/test/resources/config.properties");
modifyEnv("MMS_DEFAULT_RESPONSE_TIMEOUT_SECONDS", "130");
ConfigManager.Arguments args = new ConfigManager.Arguments();
args.setModels(new String[] {"noop_v0.1"});
ConfigManager.init(args);
ConfigManager configManager = ConfigManager.getInstance();
Assert.assertEquals("true", configManager.getEnableEnvVarsConfig());
Assert.assertEquals(130, configManager.getDefaultResponseTimeoutSeconds());
}
}