capacityConstaint will throw exception on empty domain, should it?

[reith]

Currently model.solve() with or-tools backend will throw a runtime exception if there is a capacityConstraint being evaluated on empty domain set. Is it necessary? For now library users can check domain by querying table or view that capacityConstraint applies on but they should also synchronize models data-cache and database query execution until there is an API to query from model's cached data (is it?).

Also, k8s-scheduler implementations assumes mode.solve() does not throw exceptions (otherwise scheduler execution loop will be terminated). Maybe it's better to make sure operations never throw exception or maybe just checked exceptions?

[lalithsuresh]

Thanks for the bug report @reith

Currently model.solve() with or-tools backend will throw a runtime exception if there is a capacityConstraint being evaluated on empty domain set. Is it necessary?

I agree that this should not be a generic RuntimeException. I'd prefer to treat this as a ModelException, because asking for a group of variables to be assigned values from an empty domain is trivially false. I'll push a change.

For now library users can check domain by querying table or view that capacityConstraint applies on but they should also synchronize models data-cache and database query execution until there is an API to query from model's cached data (is it?).

Could you expand on this? Is it the case that you'd like to control which tables/views updateData() pulls in? If so, you might be interested in the fetcher API (see bfdbb79).

Also, k8s-scheduler implementations assumes mode.solve() does not throw exceptions (otherwise scheduler execution loop will be terminated). Maybe it's better to make sure operations never throw exception or maybe just checked exceptions?

It does catch InterruptedException and ModelException (see Scheduler.startScheduler()). An unsatisfiable model will throw ModelException.

I had another branch where I triggered pre-emption if the initial placement attempt fails, but that's the kind of logic you'd want to execute when catching ModelException. That is, trigger some kind of fallback to make progress in your system (place a subset of tasks, kick out existing tasks, use a simplified model with fewer constraints, etc.).

[lalithsuresh]

Also worth asking given the last point: Would you like tooling to help diagnose why a ModelException happened? I could throw more specific exceptions (like UnsatModelException or InvalidInputDataException). If you'd like diagnostics into which variables/constraints were unsatisfiable, I could prioritize that too.

[reith]

I agree that this should not be a generic RuntimeException. I'd prefer to treat this as a ModelException, because asking for a group of variables to be assigned values from an empty domain is trivially false. I'll push a change.

Great! thanks.

Could you expand on this? Is it the case that you'd like to control which tables/views updateData() pulls in? If so, you might be interested in the fetcher API (see bfdbb79).

I meant to ensure capacityConstraint domain is not empty, one can query DB and result and lock database changes during that query execution and model's data update. IIUC fetcher API is about feeding model with an arbitrary data source (possibly not from db), I was seeking for the other way around; query model's cached data (after model.updateData) without sending query to DB. It may be not a very useful feature anyway.

It does catch InterruptedException and ModelException (see Scheduler.startScheduler()). An unsatisfiable model will throw ModelException.

Yes, you are right. I was just looking for checked exceptions and method signature and scopes.

Also worth asking given the last point: Would you like tooling to help diagnose why a ModelException happened? I could throw more specific exceptions (like UnsatModelException or InvalidInputDataException). If you'd like diagnostics into which variables/constraints were unsatisfiable, I could prioritize that too.

Throwing more specific exceptions is certainly useful and I think it's the prime reason exceptions can be useful to be part of API, otherwise they are not more expressive than return values. In my case it wasn't hard to find out which constraint is failing but having more contextual data in exceptions would be nice.

I close this one, thanks for the fix.