add two link attributes

[woutdenolf]

Use case: ewoks-kit/ewoksppf#162

• Added link attribute cache_if_not_required: cache inputs from non-required links just like required links.
• Handle link attribute value required=False: for the link to be explicitly non-required.
• Define in the Ewoks workflow specification that non-required and non-cached inputs are never lost
  when all required inputs are provided at any point in time.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[loichuder]

There is no implementation for cache_non_required required in ewokscore?

[woutdenolf]

There is no implementation for cache_non_required required in ewokscore

Indeed. The engine does the caching.

This is already the case for all our engines for required links. When you have two required links coming in, the task gets executed with parameters from both. This means the engine needs to cache and merge somehow.

Only now we have the new parameter it becomes clear that this is an implicit behavior of DAGs which we don't think about.

[woutdenolf]

I think I have a better solution.

Seems to have fixed a bug as well. It broke a test of this graph

flowchart TD
    start([start])
    fan([fan])

    always1([always1])
    on_true1([on_true1])
    on_error1([on_error1])

    always2([always2])
    on_true2([on_true2])
    on_error2([on_error2])

    merge([merge])
    end_always([end_always])
    end_on_error([end_on_error])

    start --> fan

    fan --> always1
    fan -->|result == true| on_true1
    fan -->|error| on_error1

    always1 --> always2
    on_true1 --> on_true2
    on_error1 --> on_error2

    always2 --> merge
    on_true2 --> merge
    on_error2 --> merge

    merge --> end_always
    on_error2 --> end_on_error

Loading

But the links always1->always2 and always2->merge are required and the test assumed they were not.

[loichuder]

Looks good!

Some naming suggestions.

[woutdenolf]

Can ewoksweb handle a boolean field that also needs to be set to null (the default)?

[woutdenolf]

Whatever we decide in ewoks-kit/ewoksppf#162 (comment) should be added to the docs.

[loichuder]

Can ewoksweb handle a boolean field that also needs to be set to null (the default)?

I think yes. There will be some head-scratching for the UI (I can't use a simple checkbox since there are three states) but the value can be serialized/deserialized as true, false or null so no technical issue.

Issue created at ewoks-kit/ewoksweb#852

[woutdenolf]

Technical discussion + decision in node execution semantics: https://confluence.esrf.fr/display/AAWWK/Ewoks+graph+analysis%3A+input+caching

Only affects the SPEC and ewoksppf.

[woutdenolf]

I keep modifying the examples. I now have two examples which I think covers all permutations of options and arrival orders.

[woutdenolf]

The main sections in the SPEC are now:

Graph definition
     Graph attributes
     Node attributes
     Link attributes
Node execution semantics
     Before the first execution
     First execution
     After the first execution
     Summary
     Example 1
     Example 2
Task scheduling
Task implementation

[woutdenolf]

I just realized that all engines other than ppf would need to refuse executing graphs with required=False. They already refuse graphs that have loops or conditions.

[woutdenolf]

I introduced graph_is_not_standard_dag so that the other engines can use this and not worries about complex graphs.

I also realized that the sequential engine does not need to reject workflow with required=False. It does a topological sort of the graph so all parents are executed before their children.

[woutdenolf]

Actually no, scratch that: a workflow that does not handle the link attribute required=False can execute the graph perfectly fine. It just removes the race-conditions. I'll stop pushing changes. To be discussed in person.

Edit: this was agreed upon during the workflow meeting

• Engines that don't handle required can just ignore it
• No extra function for the engines like graph_is_cyclic and graph_has_conditional_links is needed (the engines use this to check whether that can execute a graph or not).

[woutdenolf]

@loichuder Original review suggestions were addressed and I improved the SPEC docs (including examples).

[loichuder]

I'd say it is more explanation than reference, according to diataxis but no need to delve into semantics

[woutdenolf]

Thanks for the review!

I will now adapt the associated ewoks-kit/ewoksppf#162.

After that we can merge and make a major release.

[woutdenolf]

Should we use cache_if_optional instead of cache_if_not_required?

Some sentences to see whether this could cause confusion:

• Tasks have optional and required inputs.
• Links can be optional or required.
• Links are optional because they are explicitly not required or they are conditional or any upstream link is optional.
• Links are conditional when they have conditions or on_error=True.

Edit: In the SPEC we use the word non-required so that could be optional as well. We just need to make sure that it is clear we are talking about links and not task inputs.

[loichuder]

Should we use cache_if_optional instead of cache_if_not_required?

No hard stop for me. As long as we have clear about what we are talking about and it is written clearly in the spec

[woutdenolf]

Now we talk about "optional links" instead of "not required links" or "non required links".

Added:

• Make clear that optional/required of links and task inputs are unrelated concepts
• Make clear that the caching is per link