Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using feature-group specific flow paths has instance model errors for the "path not taken". #1984

Closed
AaronGreenhouse opened this issue Sep 6, 2019 · 10 comments · Fixed by #2022
Closed
Assignees
Milestone

Comments

@AaronGreenhouse
Copy link
Contributor

@AaronGreenhouse AaronGreenhouse commented Sep 6, 2019

Summary

Flow path implementations can be between feature group features so as to be specify the exact route of the flow path. When the model is instantiated an end to end flow using such a flow path will follow that specific path only, and not all possible paths. This seems to be working fine. However, errors may be present on the instance model regarding the inability to connect the flow path with other incoming and outgoing connections because the flow path doesn't mention their start or end port, respectively.

Expected and Current Behavior

There should be no such such errors generated in this case because the point of specific flow path implementation is to indicate the exact path through the "feature group connection" that is of interest.

Steps to Reproduce

  1. Instantiate process top.specific
  2. You should get an instance model that has 8 semantic connections and 1 end to end flow e2e
  3. The model will have two errors attached to top_specific_instance
  • Cannot create end to end flow 'e2e' because flow 'fpath' does not connect to the start of the semantic connection 't.fg2.x_in1 -> q.fg2.x_in1'
  • Cannot create end to end flow 'e2e' because the end of the semantic connection 'q.fg1.x_in2 -> t.fg1.x_in2' does not connect to the start of flow 'fpath'

These 2 errors are not appropriate in this case.

package FGConnections
public
    feature group FG
    	features
	    	x_in1: in data port;
	    	x_in2: in data port;
	    	x_out1: out data port;
	    	x_out2: out data port;
    end FG;

	feature group FG_inverse inverse of FG
	end FG_inverse;

    subprogram sub
        features
            p1: in parameter;
            p2: out parameter;
        flows
            subPath: flow path p1 -> p2;
    end sub;
    
    thread th
    	features
    		fg1: feature group FG;
    		fg2: feature group FG_inverse;
    	flows
    		fpath: flow path fg1 -> fg2;
    end th;
    
    thread implementation th.specific
    	flows
    		fpath: flow path fg1.x_in1 -> fg2.x_in2;
    end th.specific;
        
    thread th2
    	features
    		fg1: feature group FG_inverse;
    		fg2: feature group FG;
    	flows
    		fsrc: flow source fg1;
    		fsnk: flow sink fg2;
    end th2;
    
    thread implementation th2.i
    	-- trivial
    end th2.i;
    
    process top
    end top;
    
    process implementation top.specific
        subcomponents
            t: thread th.specific;
            q: thread th2.i;
        connections
            c1: feature group q.fg1 <-> t.fg1;
            c2: feature group t.fg2 <-> q.fg2;
        flows
            e2e: end to end flow q.fsrc -> c1 -> t.fpath -> c2 -> q.fsnk;
    end top.specific;
end FGConnections;
@AaronGreenhouse
Copy link
Contributor Author

@AaronGreenhouse AaronGreenhouse commented Sep 27, 2019

So, the generated end to end flow uses feature group sub features x_in1 and x_in2, as expected based on the specific flow path implementation:

fpath: flow path fg1.x_in1 -> fg2.x_in2;

The error

Cannot create end to end flow 'e2e' because flow 'fpath' does not connect to the start of the semantic connection 't.fg2.x_in1 -> q.fg2.x_in1'

is a "problem" with the source of c2 not connecting to the end of fpath. It is the outgoing connection from the flow path that is not actually used.

The error

Cannot create end to end flow 'e2e' because the end of the semantic connection 'q.fg1.x_in2 -> t.fg1.x_in2' does not connect to the start of flow 'fpath'

is a problem with the destination of c1 not connecting to the start of fpath. It is the incoming connection to the flow path that is not actually used.

@AaronGreenhouse
Copy link
Contributor Author

@AaronGreenhouse AaronGreenhouse commented Sep 27, 2019

In CreateEndToEndFlowsSwitch.processFlowStep() line 518, need to figure treat calls to isValidContinuation() differently. Should not call this at all and just skip the connection if the connection part of a flow group connection, and the end of the flow names a specific sub feature of the feature group.

@AaronGreenhouse
Copy link
Contributor Author

@AaronGreenhouse AaronGreenhouse commented Oct 2, 2019

I've been looking at this for a while now trying to understand what the method are really doing, and trying to create a situation where here the isValidContinuation() methods generate an error message that we actually want to keep. I have been unsuccessful. They only time I think these methods are going to generate an error is when the connection comes from the feature group connection along a connection that is not interesting for the flow.

That is, I think we never actually want to output an error message from isValidContinuation(). Instead, they are correctly used to silently filter out the unwanted feature group connections.

Could use another opinion on this.

@AaronGreenhouse
Copy link
Contributor Author

@AaronGreenhouse AaronGreenhouse commented Oct 4, 2019

Okay, was finally able to produce some counter examples of sorts. If I take the example above, add a second incoming feature group to th, and connect to that feature instead of the feature used in the end to end flow, I get this:

package bad_incoming_connection
public
    feature group FG
        features
            x_in1: in data port;
            x_in2: in data port;
            x_out1: out data port;
            x_out2: out data port;
    end FG;

    feature group FG_inverse inverse of FG
    end FG_inverse;
    
    thread th
        features
            fg1: feature group FG;
            fg1_wrong: feature group FG;
            fg2: feature group FG_inverse;
        flows
            fpath: flow path fg1 -> fg2;
    end th;
    
    thread implementation th.specific
        flows
            fpath: flow path fg1.x_in1 -> fg2.x_in2;
    end th.specific;
        
    thread th2
        features
            fg1: feature group FG_inverse;
            fg2: feature group FG;
        flows
            fsrc: flow source fg1;
            fsnk: flow sink fg2;
    end th2;
    
    thread implementation th2.i
        -- trivial
    end th2.i;
    
    process top
    end top;
    
    process implementation top.specific
        subcomponents
            t: thread th.specific;
            q: thread th2.i;
        connections
            c1: feature group q.fg1 <-> t.fg1_wrong;
            c2: feature group t.fg2 <-> q.fg2;
        flows
            e2e: end to end flow q.fsrc -> c1 -> t.fpath -> c2 -> q.fsnk;
    end top.specific;
end bad_incoming_connection;

There is an error on the declarative model on c1 in the declaration of the end to end flow. If you instantiate the model, however, there is an instance of the end to end flow that uses the erroneous connection c1 There is no error regarding this situation reporter, although we still have errors for the "path not taken". But even these errors are wrong because they assume it is possible to form the flow path using c1.

The problem here is that the methods isValidContinuation(), and possibly others, only compare the end features of the connection/flow paths, they do not consider that the features many be inside a feature group.

@AaronGreenhouse
Copy link
Contributor Author

@AaronGreenhouse AaronGreenhouse commented Oct 4, 2019

If you try the same trick but with the outgoing feature group of th, things behave a bit differently. In this case no end to end flow instance is created because collectConnectionInstances() returns an empty list. There is an error on the instance model that there are no semantic connections that continue from the flow path. This is correct.

We sill have an error about a "path not taken" at the start of the end to end flow. (We don't want this.)

It seems that collectConnectionInstances() is not behaving quite the same way for going into a flow and coming out of a flow.

package bad_outgoing_connection
public
    feature group FG
        features
            x_in1: in data port;
            x_in2: in data port;
            x_out1: out data port;
            x_out2: out data port;
    end FG;

    feature group FG_inverse inverse of FG
    end FG_inverse;
    
    thread th
        features
            fg1: feature group FG;
            fg2: feature group FG_inverse;
            fg2_wrong: feature group FG_inverse;
        flows
            fpath: flow path fg1 -> fg2;
    end th;
    
    thread implementation th.specific
        flows
            fpath: flow path fg1.x_in1 -> fg2.x_in2;
    end th.specific;
        
    thread th2
        features
            fg1: feature group FG_inverse;
            fg2: feature group FG;
        flows
            fsrc: flow source fg1;
            fsnk: flow sink fg2;
    end th2;
    
    thread implementation th2.i
        -- trivial
    end th2.i;
    
    process top
    end top;
    
    process implementation top.specific
        subcomponents
            t: thread th.specific;
            q: thread th2.i;
        connections
            c1: feature group q.fg1 <-> t.fg1;
            c2: feature group t.fg2_wrong <-> q.fg2;
            
            xx: feature group t.fg2 <-> q.fg2;
        flows
            e2e: end to end flow q.fsrc -> c1 -> t.fpath -> c2 -> q.fsnk;
    end top.specific;
end bad_outgoing_connection;

@AaronGreenhouse
Copy link
Contributor Author

@AaronGreenhouse AaronGreenhouse commented Oct 4, 2019

Simpler examples that don't use feature groups. This is what I was really trying to uncover, whether the errors from isValidContinuation really make sense outside of feature groups, and I think the answer is no.

package bad_incoming_simple
public
    thread th
        features
            i: in event port;
            i_wrong: in event port;
            o: out event port;
        flows
            fpath: flow path i -> o;
    end th;
    
    thread implementation th.specific
        flows
            fpath: flow path i -> o;
    end th.specific;
        
    thread th2
        features
            o: out event port;
            i: in event port;
        flows
            fsrc: flow source o;
            fsnk: flow sink i;
    end th2;
    
    thread implementation th2.i
        -- trivial
    end th2.i;
    
    process top
    end top;
    
    process implementation top.specific
        subcomponents
            t: thread th.specific;
            q: thread th2.i;
        connections
            c1: port q.o -> t.i_wrong;
            c2: port t.o -> q.i;
        flows
            e2e: end to end flow q.fsrc -> c1 -> t.fpath -> c2 -> q.fsnk;
    end top.specific;
end bad_incoming_simple;

and

package bad_outgoing_simple
public
    thread th
        features
            i: in event port;
            o: out event port;
            o_wrong: out event port;
        flows
            fpath: flow path i -> o;
    end th;
    
    thread implementation th.specific
        flows
            fpath: flow path i -> o;
    end th.specific;
        
    thread th2
        features
            o: out event port;
            i: in event port;
        flows
            fsrc: flow source o;
            fsnk: flow sink i;
    end th2;
    
    thread implementation th2.i
        -- trivial
    end th2.i;
    
    process top
    end top;
    
    process implementation top.specific
        subcomponents
            t: thread th.specific;
            q: thread th2.i;
        connections
            c1: port q.o -> t.i;
            c2: port t.o_wrong -> q.i;
        flows
            e2e: end to end flow q.fsrc -> c1 -> t.fpath -> c2 -> q.fsnk;
    end top.specific;
end bad_outgoing_simple;

Again, in the case of bad_incoming_simple we see an error that comes from isValidContinuation, but we don't see this in the case of bad_outgoing_simple. In that case we have the error that comes from collectConnectionInstances return an empty list.

@AaronGreenhouse
Copy link
Contributor Author

@AaronGreenhouse AaronGreenhouse commented Oct 4, 2019

So revisiting my comment from October 2, I think the right thing to do is

  1. Fix collectConnectionInstances() to work properly for connections going into a flow.
  2. Change the isValidContinuation() methods to not output errors, but rather to purely be filters. They probably should get a new name to reflect the different purpose.

@AaronGreenhouse
Copy link
Contributor Author

@AaronGreenhouse AaronGreenhouse commented Oct 4, 2019

Getting very confused. I need to look at all the places where semantic connections are actually added to the end to end flow instance under construction. One such place is line 533. We aren't checking that the semantic connections actually connect.

@AaronGreenhouse
Copy link
Contributor Author

@AaronGreenhouse AaronGreenhouse commented Oct 12, 2019

Fixed this by changing processFlowStep to use isValidContinuation as a filter only. It is not used to report errors. A first pass is made through all the connection instances returned by collectConnectionInstances where we test if they are valid. Only the valid ones are used in the subsequent processing. Only if there are no valid connections do we report an error. I'm pretty sure it is actually impossible for there to be no valid connections, but the error case is there just in case.

@AaronGreenhouse
Copy link
Contributor Author

@AaronGreenhouse AaronGreenhouse commented Oct 14, 2019

Okay, I was wrong, The error case can be triggered. This was uncovered by the unit tests for Issue #879. The case happens when the connections inside the component implementation "go around" the flow implementation. It becomes the opposite case of the error that is checked at the start of processFlowStep. Here we have no connection instance that connects to the start of the flow path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants