Config management

[N-Wouda]

Closes #118. This PR:

• Introduces a configuration object, and methods to interact with it.
• The new configuration replaces manual configuration in many places.
• Adds support for file-based (TOML) configuration.

[N-Wouda]

Fairly unhappy that this has become such a huge PR, but around half of it is just data files.

[N-Wouda]

I think this is ready for a first review. I propose we support (combinations of) the following keys in a TOML file:

static [ operator definitions for the static solver / hindsight ] static.params [ params passed to hgspy.Config for the static solver / hindsight ]

dynamic [ strategy name + operator definitions for the static solver used in the dynamic case ] dynamic.params [ params passed to hgspy.Config for the static solver used in the dynamic case ] dynamic.strategy_params [ free-form fields passed to the selected dynamic strategy as kwargs ]

[leonlan]

I think this is ready for a first review. I propose we support (combinations of) the following keys in a TOML file:

static [ operator definitions for the static solver ] static.params [ params passed to hgspy.Config for the static solver ]

dynamic [ operator definitions for the static solver used in the dynamic case ] dynamic.params [ params passed to hgspy.Config for the static solver used in the dynamic case ] dynamic.strategy [ free-form fields passed to the selected dynamic strategy as kwargs ]

hindsight [ operator definitions for the static solver used by hindsight ] hindsight.params [ params passed to hgspy.Config for the static solver used by hindsight ]

I'll into into this tomorrow morning in more detail. My first thoughts regarding this:

• hindsight feels redundant to me; we can use the static configuration to solve hindsight. I suggest replacing it for simulation, i.e., the configuration to solve the simulation instances. This could then also replace [dynamic.strategy] and [dynamic.strategy.sim_config].

[N-Wouda]

• hindsight feels redundant to me; we can use the static configuration to solve hindsight

Yeah makes sense. Wasn't sure about this.

I suggest replacing it for simulation, i.e., the configuration to solve the simulation instances. This could then also replace [dynamic.strategy] and [dynamic.strategy.sim_config].

I found this hard to balance between generality and easy of use. The problem I see with your suggestion is that simulations are only part of the rollout strategy, not of the baselines (or possibly, any other strategy). Now that's OK - every strategy should fill out [dynamic.strategy] with its own preferred layout and parameters. I am not sure how we should make your suggestion work in general: what do we pass into the dynamic_strategy function from the given config object?

[leonlan]

I found this hard to balance between generality and easy of use. The problem I see with your suggestion is that simulations are only part of the rollout strategy, not of the baselines (or possibly, any other strategy). Now that's OK - every strategy should fill out dynamic.strategy with its own preferred layout. I am not sure how we should make your suggestion work in general: what do we pass into dynamic_strategy from the given config object?

I'm fine with keeping it like this for generality! I dont have other comments.

[N-Wouda]

@jaspervd96 could you have a look at this and let me know what you think?

[jmhvandoorn]

https://github.com/N-Wouda/Euro-NeurIPS-2022/blob/233bc75f9d9cc6d189f6cea4d46ffcdf41134b75/strategies/dynamic/rollout/rollout.py#L32-L34

Can we make this a parameter as well? So we can do this based on the must_dispatch percentage instead of always "skip" the first epoch.

Something like:

perc_must_dispatch = len(must_dispatch) / n_requests
    if (
        perc_must_dispatch <= config["min_perc_must_dispatch"]
        or perc_must_dispatch >= config["max_perc_must_dispatch"]
    ):
        return observation["epoch_instance"], 0

Reason: see these results

[jmhvandoorn]

https://github.com/N-Wouda/Euro-NeurIPS-2022/blob/233bc75f9d9cc6d189f6cea4d46ffcdf41134b75/strategies/dynamic/rollout/rollout.py#L32-L34

Can we make this a parameter as well? So we can do this based on the must_dispatch percentage instead of always "skip" the first epoch.

Something like:

perc_must_dispatch = len(must_dispatch) / n_requests
    if (
        perc_must_dispatch <= config["min_perc_must_dispatch"]
        or perc_must_dispatch >= config["max_perc_must_dispatch"]
    ):
        return observation["epoch_instance"]

Reason: see these results

When I think about it better, setting 0 and 1 as bounds here isn't exactly the same as what happens now. So in that case, we might want to test this separately first and find out what works best

[N-Wouda]

Can we make this a parameter as well? So we can do this based on the must_dispatch percentage instead of always "skip" the first epoch.

I think so, but I feel this PR might be not be the right place to apply those changes. Looking at #87, maybe we should do more of such relevant changes in a follow-up PR?

[jmhvandoorn]

I'll only be able to reply on this later

[N-Wouda]

@jaspervd96 when's later?

[N-Wouda]

I'm not super happy about how we pass strategy parameters into the different strategies, but I could also not think of something better. This is certainly better than how we did it before, so until we have a better idea I think we should go with this.

I'll fix the merge conflict and then merge it in.