allow addModule method that will not allow a 2nd override of the same interface

[kainagel]

I tried to code what we debated: a controler.addModule(...) method that would enforce internal consistency between modules.

The idea with this method is that: (1) one can override matsim defaults, but (2) one cannot override them a second time.

The behavior should be the same as

controler.addOverridingModule( ...
   ... install() ...
      install( new Module1() ) ;
      install( new Module2() ) ;
) ;

except that the syntax now is

controler.addModule( new Module1() ) ;
controler.addModule( new Module2() ) ;

Evidently, the main usecase is where these modules are added in different methods or classes ... we certainly have that with our hierarchical approach in Berlin, where run classes use configureControler(...) methods before adding additional material into the controler.

There is also a test class, MultipleOverridesOfSameBinding, which is testing this. I can't say how far it goes and/or if it will fail eventually.

I also can't say how it will deal with multiple overrides of the multibinders, e.g. when someone adds a disutility for the same mode again.

I am adding it as a PR for discussion ...

[mrieser]

Thanks. I was not able to actually test the changes due to temporarily missing my dev environment (my laptop is in treatment for a new battery), but by looking at the code changes it looks reasonable. Also, thanks for adding tests, they look fine and are implemented in a way as I would have done based on how I understood the problem at the DevMtg.

[kainagel]

Thanks. After having thought about this, I am now wondering if we should not make this the default. I had now one or two cases where two different contribs overwrote each other, and this indeed either goes totally unnoticed, or leaves users baffled. My intuition would be to (1) change "addOverridingModule" to that new behavior. The syntax would not be totally wrong, since it would still override the default settings for the matsim extension points. (2) add a method addOverridingModule( boolean allowMultipleOverrides, AbstractModule module ) that could be used to explicitly ask for the old behavior. (Clearly, could also have a differently named second method, but I find such a switch not so stupid for testing one version against the other.)

[jfbischoff]

To me, the phrasing

(1) change "addOverridingModule" to that new behavior.
boolean allowMultipleOverrides

sounds a little strange.

I'm a little bit afraid that this change would also break many peoples' code who rely on the API without being that much more useful.

[kainagel]

Joschka, could you please explain a bit more, I don't understand, in part because your inclusion of my material is incomplete. Questions:
(1) The new behavior would be that you could override the matsim default once, but not a second time. Is it really that so many people rely on overriding it a second time?
(2) Why does it sound so strange? It would still allow overriding, after all.

[jfbischoff]

(1) The new behavior would be that you could override the matsim default once, but not a second time. Is it really that so many people rely on overriding it a second time?

Mhm. I agree for the default modules.
The use case I have in mind is, that some additional modules, like DRT or EV, override some modules in their general setup by some (contrib-specific) defaults which are then overridden again to implement some user specific behavior. Would this be affected?

(2) Why does it sound so strange? It would still allow overriding, after all.

But what happens if we have
controler.addModule(MyModule)
followed by

controler.addOverridingModule(false, MyModule)
controler.addOverridingModule(true, MyModules)

? I'm just a little bit concerned that this could confuse people even more. But maybe it's not such an issue.

[kainagel]

I see, thanks for clarifying that. My intuition is that it would be possible to have a contrib set additional modules, and then allow user code to override them once (case 1). My intuition also is that it would be more difficult to have a contrib override an already existing core module, and then allow user code to override it again, but only once (case 2). Is case 2 an issue with drt? If not, we could think about an implementation for case 1.

[jfbischoff]

I don't think case 2 is an issue.

[sebhoerl]

I also think addOverridingModule with an additional flag looks a bit confusing. But following the discussion would this maybe be a viable way to go:

• addModule is the new standard and should always be used in contribs (i.e. non-userspace code). It simply adds a module which will lead to an error if there is a collision of bindings. And interacting contribs should be designed such that they do not override each others things?
• addOverridingModule can be used by the user once to draft things and do experiments.

The question would be what does once mean. Either throw and error at the second attempt or print a clear warning that this can be dangerous.

@jfbischoff What would be a concrete use case of "case 1" where DRT overrides some default bindings?

[kainagel]

In several of our setups, we need addOverridingModule(...) at more than one level, so I would like to keep that as an option at least for the time being. I could probably live with a warning. I could probably even live with a config switch where such behavior explicitly needs to be switched on.

[michalmac]

What would be a concrete use case of "case 1" where DRT overrides some default bindings?

I think the default DRT configuration does not need any overriding at all (it does not override the core matsim). However, e-DRT overrides two EV bindings, but that should be easy to change.

As for limiting the number of overriding modules, I suggest some transition period when this limit is not imposed, so that we can gradually get rid of overriding modules and then see what is left.