Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.58%. Comparing base (05c565c) to head (19b02b1).
Report is 4 commits behind head on master.

@@            Coverage Diff             @@
##           master    #2110      +/-   ##
==========================================
- Coverage   86.60%   86.58%   -0.03%     
==========================================
  Files          90       90              
  Lines       10282    10284       +2     
==========================================
- Hits         8905     8904       -1     
- Misses       1377     1380       +3     

@PProfizi To answer the question in the description, the original problem is that before we were crashing in the kernel and not raisin in Python, so in any way the connection with wrong labels was not warking (the process couldn't be run end-to-end). Therefore, why having a new method?

@PProfizi To answer the question in the description, the original problem is that before we were crashing in the kernel and not raisin in Python, so in any way the connection with wrong labels was not warking (the process couldn't be run end-to-end). Therefore, why having a new method?

Because weirdly it did not crash all the time, for example in PyDPF-Post's tests I have to update calls because workflow connections with unavailable inputs or outputs are done all the time, without raising and without crashing.
I think the case where it crashes is a specific case.
Which is why this ends-up being a mix between a bug fix and a breaking change in behavior.

Just to sum-up:

• initial reported problem is that Workflow.connect_with() sometimes crashes the server due to unavailable inputs/outputs on either workflow being connected -> instead we should check for that in Python before the server call, and raise an explicit and helpful Python ValueError.
• This however started raising errors in PyDPF-Post where calls to Workflow.connect_with() which used to work now raise the error, meaning they did not crash the server despite unavailable inputs/outputs -> this means the change is not only a bug fix but also a breaking change for cases where it used to work and may have to be treated as such
• A solution is to filter the inputs/outputs to connect given to the method, and only perform connections for available ones. This way instead of raising an error we filter and clean the dict for the user -> this is however hidden from the user who may not understand why a connection is not made. Should we then raise a warning? These are going to be raised a lot.
• This is why another solution is to have two distinct methods: one which filters for the user, and one which raises if something is unavailable. -> we could also add a new option to Workflow.connect_with() which switches between filtering and raising.

Just to sum-up:

• initial reported problem is that Workflow.connect_with() sometimes crashes the server due to unavailable inputs/outputs on either workflow being connected -> instead we should check for that in Python before the server call, and raise an explicit and helpful Python ValueError.
• This however started raising errors in PyDPF-Post where calls to Workflow.connect_with() which used to work now raise the error, meaning they did not crash the server despite unavailable inputs/outputs -> this means the change is not only a bug fix but also a breaking change for cases where it used to work and may have to be treated as such
• A solution is to filter the inputs/outputs to connect given to the method, and only perform connections for available ones. This way instead of raising an error we filter and clean the dict for the user -> this is however hidden from the user who may not understand why a connection is not made. Should we then raise a warning? These are going to be raised a lot.
• This is why another solution is to have two distinct methods: one which filters for the user, and one which raises if something is unavailable. -> we could also add a new option to Workflow.connect_with() which switches between filtering and raising.

Thanks for summarizing everything so clearly. I think that the followed approach (with the 2 methods) is the right one giving all the compatibility restrictions

@rafacanton I ended-up adding the permissive parameter with a default value of True which I think is the graceful way of solving this.
I think most people who will want to use permissive=False are developers and not users.