Skip to content

feat: Create profile set custom seed - #2665

Open
diogodanielsoaresferreira wants to merge 24 commits into
TimefoldAI:mainfrom
diogodanielsoaresferreira:create_profile_set_custom_seed
Open

diogodanielsoaresferreira wants to merge 24 commits into
TimefoldAI:mainfrom
diogodanielsoaresferreira:create_profile_set_custom_seed

Conversation

@diogodanielsoaresferreira

Copy link
Copy Markdown
Contributor
  • Add execution profile to set random seed in run (using the var QUARKUS_TIMEFOLD_SOLVER_DEFAULT_RANDOM_SEED)
  • Add possibility to add parameters to execution profiles

Fixes: https://github.com/TimefoldAI/timefold-platform/issues/5335

if (seed == null) {
return Map.of();
}
return Map.of(ENV_QUARKUS_RANDOM_SEED, seed);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the expectation is that gateway will call this method before scheduling pod and set these variables on pod spec?

Maybe we could consider a custom implementation of ConfigSource that we could back it by the configuration from storage - that way we could load execution profile and the options in that config source which would then deliver anything that quarkus config properties might expect.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is good, and we could read it from storage, but the problem is that ConfigSources initialize very early in the lifecycle, and the storage clients are configured via CDI, so we would have to connect to the storages manually, which would be fickly.

Another option would be to rely on a configurations file mounted by the gateway that could be read by the pod. But that would again increase the complexity.

As I think that this options works for now and it's easier to maintain, I would keep it as is for now.

/**
* Runs the solver with a fixed random seed, making a run reproducible.
* <p>
* The seed is an optional {@code seed} parameter; when it is not supplied, a random seed is generated and persisted with

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the seed generation depends on the environment mode. By default, we use the PHASE_ASSERT mode, which has a seed value of 0. In contrast, the NON_REPRODUCIBLE mode uses a random number as its seed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that means that for this to be useful, we also need to set the environment mode to NON_REPRODUCIBLE?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it just means the seed 0 will be used with the default environment. But I see you already updated the documentation, which makes more sense to me now. I believe we want to keep the runs reproducible wherever possible, and we can now change the seed if necessary. Using NON_REPRODUCIBLE does not seem necessary.

@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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we pass arbitrary values to options?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but only the needed values will be used by the Execution Profile; other ones will be discarded

assertThat(runNode.isObject()).as("run configuration should be serialized as an object").isTrue();
((ObjectNode) runNode).put("notARealOption", "boom");

// Permitting unknown properties in the schema must not weaken the strict Jackson mapping.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test answers my previous question in RunConfiguration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants