[Refactoring step 2] Introduce generic static solver

[jmhvandoorn]

Based on issue #114 this introduces a generic static solver and the possibility to add N many different solver configurations.

I believe it includes all remarks / suggestions / comments that have been made here, but feel free to add more :)

[N-Wouda]

Can we do the config/toml stuff in a follow-up PR? I'd rather get this to work well first and then think about config management second.

[N-Wouda]

I've added the relevant cpp changes. The Result object now makes a copy of the passed-in best individual.

[jmhvandoorn]

Can we do the config/toml stuff in a follow-up PR? I'd rather get this to work well first and then think about config management second.

How would you propose to split this?

I agree the config management is not final as is now, but if we introduce this generic solver builder, it has to get it's configuration from somewhere?

[N-Wouda]

How would you propose to split this?

Hardcode the configuration in the call site. Not too different from what we used to do.

[jmhvandoorn]

How would you propose to split this?

Hardcode the configuration in the call site. Not too different from what we used to do.

But then also pass the full 3 lists of operators from the callside you mean?

[N-Wouda]

How would you propose to split this?

Hardcode the configuration in the call site. Not too different from what we used to do.

But then also pass the full 3 lists of operators from the callside you mean?

Yes. Or at least their names, or the class/functions.

[N-Wouda]

This looks OK to me. Do you want to start integrating it into the relevant call sites?

[jmhvandoorn]

@N-Wouda so this is the issue I meant earlier with passing solver parameters as kwargs.

https://github.com/N-Wouda/Euro-NeurIPS-2022/blob/ed406b8d13690461dc698fa91b033da195654061/benchmark.py#L64

Here we use kwargs to accept either max_iterations or max_runtime. However, kwargs also contains attributes that are not meant for the solver, such as "instance_pattern" and "phase". That's why you cannot pass kwargs to hgspy.Params in the way we use it now

[N-Wouda]

However, kwargs also contains attributes that are not meant for the solver, such as "instance_pattern" and "phase". That's why you cannot pass kwargs to hgspy.Params in the way we use it now

But which arguments here are actually useful for the solver? As far as I can tell, this entire **kwargs argument here does not add much.

[jmhvandoorn]

However, kwargs also contains attributes that are not meant for the solver, such as "instance_pattern" and "phase". That's why you cannot pass kwargs to hgspy.Params in the way we use it now

But which arguments here are actually useful for the solver? As far as I can tell, this entire **kwargs argument here does not add much.

max_runtime or max_iterations

[N-Wouda]

Then pop that off and pass the relevant one in? This is why I think we should just pass the stopping criterion in:

if max time in kwargs:
  stop = hgspy.stop.MaxRuntime(kwargs value)
else:
  stop = ... iterations

res = solve(..., stop)

[jmhvandoorn]

Then pop that off and pass the relevant one in?

So which one is the relevant one, depends on the input. We could fix that, but I am not in favor of refactoring all the code around it a lot, when we eventually will remove it again and pass in a config object instead.

[jmhvandoorn]

Could we maybe finish this PR without the solver being actually used? Then continue on getting a configuration management working. And only integrate it after that, to prevent double work?

[N-Wouda]

We haven't yet figured out how the actual method should be called and what its signature should be. Right now it does not work, and the first case where we wanted to integrate it failed. So we haven't yet figured out what this generic solver should look like. Until we do, I'm not inclined to approve this PR.

I don't think this has to take a lot more time: I suspect it's as simple as building the stopping criterion outside solve, and passing that criterion in. So, basically this:

I'm still on the fence about replacing max_runtime and max_iterations with stop, and leaving it to the call site to figure this one out.

[jmhvandoorn]

What about:

def solve(
  instance,
  config,
  *,
  max_runtime=None,
  max_iterations=None,
  seed=1,
  init_sols=()
)

Where config is an object of a class Config that contains 3 lists of operators and a dict of solver params?

[jmhvandoorn]

We haven't yet figured out how the actual method should be called and what its signature should be. Right now it does not work, and the first case where we wanted to integrate it failed. So we haven't yet figured out what this generic solver should look like. Until we do, I'm not inclined to approve this PR.

I get that this one is not done yet. I meant just focusing on everything not-at-call side first. And then adjust the callside to this, instead of trying to work the callside around a non-definitive solver.

[N-Wouda]

Right now I don't know if this works. We've certainly never used in in the CI, so I have no way to verify. I'm not sure what the difficulty is with trying to make this work first, before we move on to changing how parameters are stored and managed at some unrelated place in the code. Those are two fairly distinct PRs, since the solver we are currently working on should not have to know about config/config files.

What about:

def solve(
  instance,
  config,
  *,
  max_runtime=None,
  max_iterations=None,
  seed=1,
  init_sols=()
)

Where config is an object of a class Config that contains 3 lists of operators and a dict of solver params?

This might very well happen. Or it could contain the stopping criterion as well. Or we do not do this at all, and have the configuration class provide the arguments to the solver function so that it does not have to know about our config management at all. My point is that I do not know. I'd much rather figure one thing out first (the solver), before we move on to the second (config management), rather than making this into either a non-functional PR, or a PR that's doing multiple things at once.

[N-Wouda]

I don't really know how to proceed, given our current deadlock :-). @leonlan what do you think?

[leonlan]

I don't really know how to proceed, given our current deadlock :-). @leonlan what do you think?

I don't have time any more today to look at this in detail. If there's still a deadlock tomorrow morning at 7:30AM, then I'll let you guys know what I think :-)

[jmhvandoorn]

I have done a "proposal-by-implementation". Let me know what you think.

• I tried to include the config management as you proposed. (Note that for now I did not yet change when the configuration is loaded/read from file. So that's still done way too often, but can easily be changed by moving the creation of config objects to another scope (or cache them by filename).
• I did integrate the solver into the four scripts and tested that they work
• I did not yet do anything with stop. Let's keep that for a later discussion

(Yes, this PR now has the solver function and configuration management both, but I am still convinced that it is not worth our time to try to get the former working without the latter.)

[jmhvandoorn]

I handled most of the remarks above. I think those that are left, are related to a single decision we have to take first.

Do we want instances of the Config class to be "mutable"?

If not:

• We should have a single initialization that handles a mix of input from a file and keyword arguments.
• We can create a hgspy.Config instance and lookup hgspy operators in the config initialization.
• For every configuration set we want to use, we should create a new config instance.
• We should probably leave the seed and stopping criteria out of the config file, to be able to change them easier.

If yes:

• We should only store values and operator names in the config, and leave the creation and lookup of hgspy instances and functions to the solver.
• We could put the seed and stopping criteria inside the config class, and adapt them during the run.

I think I would prefer the first one, since it leads to cleaner code. Eventually, we won't change parameters during a run anyway, so we can "release" the solver of the class creation and lookups.

(note: either way, I got rid of the kwargs argument in static.solve, that catched any unused arguments passed to static.solve, by explicitly only passing the kwarg values containing seed, max_runtime and max_iterations instead of all kwargs)

[N-Wouda]

Do we want instances of the Config class to be "mutable"?

That does seem to be convention, yes. But I think a deeper question is the following: how do we want to get data out of Config objects? I think via methods, which can take the raw configuration settings as they currently exist, and turn those into the appropriate values when the method is called. E.g.

class Config(dict):
  ...

  def get_node_ops(self):
    return [hgspy.operators.<something> for something in self["node_ops"]]

The moment that's called is when it matter what's in self["node_ops"]. This has little to do with disallowing mutability or "We should have a single initialization that handles a mix of input from a file and keyword arguments." depending on perceived mutability - if we go for a dict-like class with some extras, all this works out fine.

[jmhvandoorn]

def get_node_ops(self):
    return [hgspy.operators.<something> for something in self["node_ops"]]

With what I said, I also had in might that we wanted to decide if the solver should repeat this "for loop" for every solve, or that it was done once within the initialization of the configuration. (which I think is not really important, since it will be fast).

how do we want to get data out of Config objects? I think via methods

Yes, I think either methods like that, or not at all and let the solver take care of that (like it is at this moment). For me the main advantage of the latter, is that only the solve has to care about hgspy and its classes/functions etc.

Do you intend to pass an entire config object into the solver and use those methods there? Or to use the config methods to put into arguments of the solver method?

[N-Wouda]

@jaspervd96 is there anything in this PR we would still like to merge? If not, shall we close it?

[leonlan]

I'm closing this PR. Feel free to re-open if relevant.