disable automatic selection of subclasses

[goldhoorn]

B is a component, subclassing A

At a certain point in time, an instance of B was running. In the following network A is requested but B is still-re-used even it's an instance of a different type.

See for details regarding the inheritance: https://github.com/rock-control/control-orogen-auv_control/pull/1

[doudou]

For the record, I'm copy/pasting some relevant information I've already given in https://github.com/rock-control/control-orogen-auv_control/pull/1

On the inheritance: the syskit bug is due to a behaviour that I definitely DID NOT want to have, but got pushed by the RIMRES people a while ago because it was breaking their system. It basically makes syskit think that any subclass is good to implement a superclass.

[doudou]

As far as I remember, the use case in RIMRES was that one would have an abstract superclass that is implemented differently on various platforms. That is probably the keyword, actually "abstract". It is a fine behaviour when the base is abstract and horrible when it is not.

[goldhoorn]

@2maz : could you give a statement here?

I think we should not do automatic child-selection in general, but make it able to manually define that a given child fullfills the same service than the parent. But only explicitly on the model-level.

[doudou]

model-level.

You mean profile-level ?

In general, one can already do at the profile level use ParentClasss => SubClass

[goldhoorn]

I don't know if this solves the issue for RIMES because it defines only ONE solution...

i thougt on:

SubClass.fullfills(ParentClasss) So that a automatic resolution COULD choose the SubClass but does not have to... (if it is ambigious then raise as normal)

Best, Matthias

On 05.11.2014 17:00, Sylvain Joyeux wrote:

model-level.
You mean profile-level ?

In general, one can already do at the profile level use ParentClasss => SubClass

— Reply to this email directly or view it on GitHub https://github.com/rock-core/tools-syskit/issues/36#issuecomment-61830017.

[2maz]

Not sure whether this 'bug/feature' is still relevant for the old RIMRES codebase, but it would help me to understand why breaking with an OO design principle that the subclass should apply in the context of the superclass. If this assumption does not hold, then wouldn't this rather point to a modelling fault, e.g. such as already pointed out, missing to use an additional / abstract class or introducing a corresponding dataservice?

[goldhoorn]

Just had a discussion with thomas, the result is: Szenario: class P(arent) < C(hild)

Due to the OOP principle C is a valid replacement for P, to the current Syskit behaviour is (partwise) correct. The problem is a non-yielding when generating the independand network which got merged into a running one, explanation:

class Cmp < Syskit::Composition
   add P, :as => 'parent'
end
class EarlierExecutedCmp < Syskit::Composition
  add C, :as => 'child'
end

When EarlierExecutedCmp WAS running, got stopped and Cmp got stared in one cycle, Syskit chooses C as a valid replacement for P, and C i reused. Which was not intended by my because my modeling was bad. (My Modelling does not follow the OOP principle).

But te be fair, such things happen, the solution from Syskit points of view is: P and C are both valid for add P, :as => 'parent' therefor syskit should have yielded here that P is ambiguous. From OOP point of view this is the case. Syskit should not have directly decided that P is the right decision for P (which sounds strange but this is the case here). Therefore syskit should check for available child-classes and if they are available yield and not accept this statement.

A possible layout could be something like add P.direct, :as => 'parent' or add P.use(P) :as => 'parent'

[doudou]

C is a valid replacement for B does NOT get broken. You can still select C in place of a B.

However, the problem is that currently syskit would allow itself to replace a B (on the designed network) by a C in some situations, which breaks syskit's principle of "the network should run as designed"

[goldhoorn]

ehm what is B, i assume you mean P ?!

But C could automatically choosen for P if there is no P availble (e.G. as deployment, or if its virtual) so C is automatically right.

And the problem is again that Syskit should have been raised on the verification time for the generated network of Cmp. (as explained).

[goldhoorn]

Just recognized i mixed "yield" with "yell" (aka raise). Sorry

[doudou]

ehm what is B, i assume you mean P ?!

Right. Sorry.

But C could automatically choosen for P if there is no P availble (e.G. as deployment, or if its virtual) so C is automatically right.

C could automatically be chosen for P if it is the only possible choice ever on this particular system. Yes.

And the problem is again that Syskit should have been raised on the verification time for the generated network of Cmp. (as explained).

MMmmm ... You'll have to rephrase this.

The issue it touches (and that is on my pile of "syskit pain points") is that currently deployments are selected in the robot config while everything else about the system is done in the profiles. I am thinking for a while about declaring deployments in the profiles as well to unify all of this.

[goldhoorn]

On 06.11.2014 16:10, Sylvain Joyeux wrote:

And the problem is again that Syskit should have been raised on
the verification time for the generated network of Cmp. (as
explained).

MMmmm ... You'll have to rephrase this.

Syskit should have warned when a network with Cmp get instanciated, because there were two-possibilities for P, P itself and C so the problem here is the model-checking.

P.S. btw. P and C are TaskContexts (-- be specifiv the representation --)in my case

[doudou]

There are no ambiguities at design time. If you a composition uses P and there is a deployment for P, then that composition can be instanciated without errors. If there are no deployments for P then C could be automatically selected (but if it is not it's not a big deal) and if C is explicitly selected then that's fine too. There is nothing to make an error/warning about at design time.

My guess is that the bug is either in merging (where syskit would do merge(P,C)->C and should not) or at deployment time, where syskit would select an already deployed C while the designed network has P. The second is more likely given the symptoms of having different behaviour when there is a C is already running vs. where there is no C already running.

As far as I remember, the issue is at network-deployment time, not network-design time. In other words, P was properly selected for the network. The issue is that at deployment time, syskit believes that picking C as a deployment for P is OK (it does so because there is already C in the network). Which is why you have the problem

In any case, I'll fix this after orogen_loaders gets into master.

[goldhoorn]

On 07.11.2014 16:56, Sylvain Joyeux wrote:

There are no ambiguities at design time. If you a composition uses P and there is a deployment for P, then that composition can be instanciated without errors. If there are no deployments for P then C could be automatically selected (but if it is not it's not a big deal) and if C is explicitly selected then that's fine too. There is nothing to make an error/warning about at design time.

There are ambiguities during design time!

If a composition uses P and there are deployments for P and C it IS ambigious, but syskit does not tell and THIS is the problem.

My guess is that the bug is either in merging (where syskit would do merge(P,C)->C and should not) or at deployment time, where syskit would select an already deployed C while the designed network has P. The second is more likely given the symptoms of having different behaviour when there is a C is already running vs. where there is no C already running.

I assume the same that this is the case, but Cmp with P should not have been builded because it is ambigious, because for P C IS a valid replacement.

As far as I remember, the issue is at network-deployment time, not network-design time. In other words, P was properly selected for the network. The issue is that /at deployment time/, syskit believes that picking C as a deployment for P is OK (it does so because there is already C in the network). Which is why you have the problem

Yeah this is the case, but the solution you want to do is wrong.

In any case, I'll fix this after orogen_loaders gets into master.

— Reply to this email directly or view it on GitHub https://github.com/rock-core/tools-syskit/issues/36#issuecomment-62165356.

[doudou]

If a composition uses P and there are deployments for P and C it IS ambigious, but syskit does not tell and THIS is the problem.

We disagree here.

If a definition uses P and P is not abstract then either C or a deployment for C should never be selected automatically. Ever. It is NOT a valid automatic replacement. If P can be deployed, then P should be deployed. If syskit browse shows a P, then P it must deploy.

If P is abstract, it becomes more alike to a service in which case the service rules apply. I.e. any subclass for which there are deployments are replacement candidates, and if the result is unambiguous it can be injected automatically.

If P has no deployment, I believe that an argument could be made that it would be then possible to pick C (assuming that C has indeed a deployment). However, for the sake of consistency with the behaviour when both P and C are deployed I would personally avoid doing it automatically.

Sylvain

[goldhoorn]

If a definition uses P and P is not abstract then either C or a deployment for C should never be selected automatically. Ever. It is NOT a valid automatic replacement. If P can be deployed, then P should be deployed. If syskit browse shows a P, then P it must deploy.

I fully and absolutly agree here, if syskit browse shows P then P has to be deployed! But you said it should not selected automatically, i agree, but i think it would be better to raise that there IS a ambigiuity, instead simply using P if both are availible.

My discussion point is, if there is another C in the network available, IF a merge should happen then i say it becomes ambigious.

But i think at this point, if a C and P are within the network, syskit should raise (during browse and runtime) a ambiguous error. Because P could be replaced by C, but does not have too, and syskit is from my point of view not able to make this decision. Therefore to be sure raise a ambigious error.

You completely right it should not taken automatically, but if both are available the selection becomes ambigious.

If P has no deployment, I believe that an argument could be made that it would be then possible to pick C (assuming that C has indeed a deployment). However, for the sake of consistency with the behaviour when both P and C are deployed I would personally avoid doing it automatically.

I think it is a valid replacement in this scenario, as long no P is available. This would have been shown in instanciate and is from OOP point (http://en.wikipedia.org/wiki/Liskov_substitution_principle) a valid replacement and not-ambigious. I cannot imagine a scenario right now where this can cause trouble.

The "only" problem i image in the first part, that again some weird things might happen is multiple definitions are active, then a ambiguous warning might occur during runtime (as suggested), but we have this problem for other points too, we need to increase the verification for coordination-object in any case, but this is another "issue/enhancement"

[doudou]

Having thought about Liskov overnight, here the thing: Liskov simply does not apply here.

Counter-example: AUV ground following.

You have a ground follower component P that simply uses the echosounder. Then, you refine it into a ground follower C that also uses a forward-looking imaging sonar.

You just cannot replace P with C during merging. C has inputs that P does not have, the network of the (formerly) ground-following composition will not monitor the rest of the components that are necessary for the ground follower subsystem to work. Broken network. Bad.

Interestingly, I always wondered what was the difference between a component-based system and an object-oriented one (something Herman said since forever: they are intrinsically different). I'm happy I found one. Liskov just does not apply on a component-based system because communication "sticks out" of the components while in Liskov, one assumes that objects are self-contained (i.e. all the relationships between the object and its pairs are already contained within the instanciated object).

[doudou]

If P has no deployment, I believe that an argument could be made that it would be then possible to pick C (assuming that C has indeed a deployment). However, for the sake of consistency with the behaviour when both P and C are deployed I would personally avoid doing it automatically. I think it is a valid replacement in this scenario, as long no P is available. This would have been shown in instanciate and is from OOP point (http://en.wikipedia.org/wiki/Liskov_substitution_principle) a valid replacement and not-ambigious. I cannot imagine a scenario right now where this can cause trouble.

It is a valid replacement, but enabling the automatic replacement makes syskit behave differently in subtle ways. Not bad (the resulting system would be fine), but maybe something we want to avoid in the spirit of reducing magic.

But i think at this point, if a C and P are within the network, syskit should raise (during browse and runtime) a ambiguous error. Because P could be replaced by C, but does not have too, and syskit is from my point of view not able to make this decision. Therefore to be sure raise a ambigious error.

If you assume that the replacement of P by C is "free" (i.e. that the behaviour of the system will be unchanged), then not merging will also generate a properly working system. In this case, it is actually safer to just not merge and go on.

Apart from devices, merging is an optimization. If we had infinite CPU and memory, we could just not merge anything but devices and run all components. We would just stupidly run the exact same thing 20 times. Not merging is always safe (provided there's enough deployments).