-
Notifications
You must be signed in to change notification settings - Fork 224
feat: Allow custom options in the RunConfiguration #2590
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| package ai.timefold.solver.service.definition.api.domain; | ||
|
|
||
| import java.util.Map; | ||
| import java.util.Set; | ||
|
|
||
| import jakarta.validation.constraints.Positive; | ||
|
|
@@ -11,7 +12,6 @@ | |
|
|
||
| import com.fasterxml.jackson.annotation.JsonInclude; | ||
|
|
||
| @Schema(additionalProperties = Schema.False.class) | ||
| public record RunConfiguration( | ||
| @Schema(nullable = true, | ||
| description = "Optional name to be given to the dataset. If not provided, the name will be generated.") @Size( | ||
|
|
@@ -21,13 +21,11 @@ public record RunConfiguration( | |
| description = "Optional maximum number of threads to be used for solving.", | ||
| minimum = "1") @JsonInclude(JsonInclude.Include.NON_EMPTY) @Positive Integer maxThreadCount, | ||
| @JsonInclude(JsonInclude.Include.NON_NULL) @Schema( | ||
| description = "Optional tags to be assigned to the dataset.") @Size(max = 100) Set<String> tags) { | ||
| description = "Optional tags to be assigned to the dataset.") @Size(max = 100) Set<String> tags, | ||
| @JsonInclude(JsonInclude.Include.NON_NULL) @Schema(hidden = true) Map<String, String> options) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to keep it open for any parameters? If we want to configure a custom seed, it will probably originate from the platform configuration profile, but we will need to set If we make change it to a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Idea was to keep it open to allow for other customization in the future. Other options that wouldnt be public available. I think if we add a |
||
|
|
||
| public RunConfiguration(String name, SolverTerminationConfig termination, Integer maxThreadCount, Set<String> tags) { | ||
| this.name = name; | ||
| this.termination = termination; | ||
| this.tags = tags; | ||
| this.maxThreadCount = maxThreadCount; | ||
| this(name, termination, maxThreadCount, tags, null); | ||
| } | ||
|
|
||
| public RunConfiguration(String name, SolverTerminationConfig termination) { | ||
|
|
@@ -49,14 +47,15 @@ public RunConfiguration(String name) { | |
| * @return a copy of this instance with given termination, never null | ||
| */ | ||
| public RunConfiguration withTermination(SolverTerminationConfig termination) { | ||
| return new RunConfiguration(name(), termination, maxThreadCount(), tags()); | ||
| return new RunConfiguration(name(), termination, maxThreadCount(), tags(), options()); | ||
| } | ||
|
|
||
| public RunConfiguration override(RunConfiguration configuration) { | ||
| String finalName = name; | ||
| SolverTerminationConfig finalTermination = termination; | ||
| Integer finalMaxThreadCount = maxThreadCount; | ||
| Set<String> finalTags = tags; | ||
| Map<String, String> finalOptions = options; | ||
|
|
||
| if (configuration == null) { | ||
| return this; | ||
|
|
@@ -70,16 +69,20 @@ public RunConfiguration override(RunConfiguration configuration) { | |
| finalMaxThreadCount = configuration.maxThreadCount(); | ||
| } | ||
|
|
||
| if (finalOptions == null) { | ||
| finalOptions = configuration.options(); | ||
| } | ||
|
|
||
| if (finalTermination == null) { | ||
| finalTermination = configuration.termination(); | ||
| } else { | ||
| finalTermination = finalTermination.override(configuration.termination()); | ||
| } | ||
|
|
||
| if ((finalTags == null || !finalTags.isEmpty()) && configuration.tags() != null && !configuration.tags().isEmpty()) { | ||
| finalTags = configuration.tags; | ||
| if ((finalTags == null || finalTags.isEmpty()) && configuration.tags() != null && !configuration.tags().isEmpty()) { | ||
| finalTags = configuration.tags(); | ||
| } | ||
|
|
||
| return new RunConfiguration(finalName, finalTermination, finalMaxThreadCount, finalTags); | ||
| return new RunConfiguration(finalName, finalTermination, finalMaxThreadCount, finalTags, finalOptions); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,150 @@ | ||
| package ai.timefold.solver.service.definition.api.domain; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| import java.time.Duration; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
|
|
||
| import ai.timefold.solver.service.definition.api.termination.SolverTerminationConfig; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import com.fasterxml.jackson.core.JsonProcessingException; | ||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
|
|
||
| class RunConfigurationTest { | ||
|
|
||
| private final ObjectMapper mapper = new ObjectMapper(); | ||
|
|
||
| @Test | ||
| void convenienceConstructorsLeaveOptionsNull() { | ||
| assertThat(new RunConfiguration("dataset", null, 4, Set.of("a")).options()).isNull(); | ||
| assertThat(new RunConfiguration("dataset", null).options()).isNull(); | ||
| assertThat(new RunConfiguration(4, null).options()).isNull(); | ||
| assertThat(new RunConfiguration("dataset").options()).isNull(); | ||
| } | ||
|
|
||
| @Test | ||
| void overrideFillsMissingOptionsFromFallback() { | ||
| RunConfiguration primary = new RunConfiguration("dataset", null, null, Set.of(), null); | ||
| RunConfiguration fallback = new RunConfiguration(null, null, null, Set.of(), Map.of("solver", "fast")); | ||
|
|
||
| RunConfiguration merged = primary.override(fallback); | ||
|
|
||
| assertThat(merged.options()).containsExactlyEntriesOf(Map.of("solver", "fast")); | ||
| } | ||
|
|
||
| @Test | ||
| void overrideKeepsPrimaryOptionsWhenPresent() { | ||
| RunConfiguration primary = new RunConfiguration("dataset", null, null, Set.of(), Map.of("solver", "accurate")); | ||
| RunConfiguration fallback = new RunConfiguration(null, null, null, Set.of(), Map.of("solver", "fast")); | ||
|
|
||
| RunConfiguration merged = primary.override(fallback); | ||
|
|
||
| // Options are replaced wholesale, never merged key-by-key. | ||
| assertThat(merged.options()).containsExactlyEntriesOf(Map.of("solver", "accurate")); | ||
| } | ||
|
|
||
| @Test | ||
| void overrideKeepsPrimaryOptionsWhenPresentWithDisjointFallbackKeys() { | ||
| RunConfiguration primary = new RunConfiguration("dataset", null, null, Set.of(), Map.of("solver", "accurate")); | ||
| RunConfiguration fallback = new RunConfiguration(null, null, null, Set.of(), Map.of("logLevel", "debug")); | ||
|
|
||
| RunConfiguration merged = primary.override(fallback); | ||
|
|
||
| assertThat(merged.options()).containsExactlyEntriesOf(Map.of("solver", "accurate")); | ||
| } | ||
|
|
||
| @Test | ||
| void overrideKeepsPrimaryEmptyOptionsInsteadOfInheriting() { | ||
| RunConfiguration primary = new RunConfiguration("dataset", null, null, Set.of(), Map.of()); | ||
| RunConfiguration fallback = new RunConfiguration(null, null, null, Set.of(), Map.of("solver", "fast")); | ||
|
|
||
| RunConfiguration merged = primary.override(fallback); | ||
|
|
||
| // Only a null options map inherits from the fallback; an empty one is a deliberate "no options". | ||
| assertThat(merged.options()).isEmpty(); | ||
| } | ||
|
|
||
| @Test | ||
| void overrideWithNullConfigurationKeepsOptions() { | ||
| RunConfiguration primary = new RunConfiguration("dataset", null, null, Set.of(), Map.of("solver", "fast")); | ||
|
|
||
| assertThat(primary.override(null).options()).containsExactlyEntriesOf(Map.of("solver", "fast")); | ||
| } | ||
|
|
||
| @Test | ||
| void withTerminationPreservesOptions() { | ||
| RunConfiguration configuration = | ||
| new RunConfiguration("dataset", null, 4, Set.of("nightly"), Map.of("solver", "fast")); | ||
|
|
||
| RunConfiguration copy = configuration.withTermination(new SolverTerminationConfig(Duration.ofMinutes(1), null)); | ||
|
|
||
| assertThat(copy.options()).containsExactlyEntriesOf(Map.of("solver", "fast")); | ||
| assertThat(copy.termination().spentLimit()).isEqualTo(Duration.ofMinutes(1)); | ||
| assertThat(copy.name()).isEqualTo("dataset"); | ||
| assertThat(copy.maxThreadCount()).isEqualTo(4); | ||
| assertThat(copy.tags()).containsExactly("nightly"); | ||
| } | ||
|
|
||
| @Test | ||
| void deserializesOptionsFromJson() throws JsonProcessingException { | ||
| String json = """ | ||
| { | ||
| "name": "dataset", | ||
| "options": { | ||
| "solver": "fast", | ||
| "logLevel": "debug" | ||
| } | ||
| } | ||
| """; | ||
|
|
||
| RunConfiguration configuration = mapper.readValue(json, RunConfiguration.class); | ||
|
|
||
| assertThat(configuration.name()).isEqualTo("dataset"); | ||
| assertThat(configuration.options()) | ||
| .containsExactlyInAnyOrderEntriesOf(Map.of("solver", "fast", "logLevel", "debug")); | ||
| } | ||
|
|
||
| @Test | ||
| void omitsNullOptionsFromJson() throws JsonProcessingException { | ||
| String json = mapper.writeValueAsString(new RunConfiguration("dataset")); | ||
|
|
||
| assertThat(json).doesNotContain("options"); | ||
| } | ||
|
|
||
| @Test | ||
| void serializesOptionsWhenPresent() throws JsonProcessingException { | ||
| RunConfiguration configuration = | ||
| new RunConfiguration(null, null, null, null, Map.of("solver", "fast")); | ||
|
|
||
| String json = mapper.writeValueAsString(configuration); | ||
|
|
||
| assertThat(json).contains("\"options\":{\"solver\":\"fast\"}"); | ||
| } | ||
|
|
||
| @Test | ||
| void overrideFillsNullTagsFromFallback() { | ||
| RunConfiguration primary = new RunConfiguration("dataset", null, null, null, null); | ||
| RunConfiguration fallback = new RunConfiguration(null, null, null, Set.of("nightly"), null); | ||
|
|
||
| assertThat(primary.override(fallback).tags()).containsExactly("nightly"); | ||
| } | ||
|
|
||
| @Test | ||
| void overrideFillsEmptyTagsFromFallback() { | ||
| RunConfiguration primary = new RunConfiguration("dataset", null, null, Set.of(), null); | ||
| RunConfiguration fallback = new RunConfiguration(null, null, null, Set.of("nightly"), null); | ||
|
|
||
| assertThat(primary.override(fallback).tags()).containsExactly("nightly"); | ||
| } | ||
|
|
||
| @Test | ||
| void overrideKeepsPrimaryTagsWhenPresent() { | ||
| RunConfiguration primary = new RunConfiguration("dataset", null, null, Set.of("adhoc"), null); | ||
| RunConfiguration fallback = new RunConfiguration(null, null, null, Set.of("nightly"), null); | ||
|
|
||
| assertThat(primary.override(fallback).tags()).containsExactly("adhoc"); | ||
| } | ||
| } |
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.
If it's hidden from the schema, it cannot be sent from the client, since the platform gateway will consider it an invalid parameter, correct?
So we allow these options to only be sent inside the cluster.
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.
the idea is that it can still be part of the POST body, so it wont fail, but only the Platform would know how to handle it. Sort of hidden parameter to be used.
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.
I have slightly different view, shall we have a short meeting tomorrow or on Monday three of us (@cristianonicolai @rsynek )?