search

ICRAR / daliuge

The DALiuGE Execution Engine
GNU Lesser General Public License v2.1
24 stars 7 forks source link

Improve support for keyword-positional arguments #254

Closed myxie closed 5 months ago

myxie commented 5 months ago

Issue

Previously, we addressed the issue that the positional parameter option currently does not work for Docker and Bash applications; a temporary fix was introduced in #248.

Unfortunately, that solution did not allow for the potential Keyword-and-Positional argument combination, which is used in the PyFunc DROP classes. By commenting out the addition to portargs to fix positional arguments for Docker and Bash apps, I inadvertently broke the PyFunc apps.

Solution

I have added a flag to the identify_named_ports methods that allows for the addition of positional arguments to the keyword argument dictionary, which required for the PyFunc drops to work.

I have also added some support functions to reduce the incidence of duplicated coded in replace_named_ports, to improve readability and make a bit more explicit that we are doing a lot of in-place manipulation of dictionaries (as opposed to modifying and returning them).

coveralls commented 5 months ago

Coverage Status

coverage: 79.657% (-0.07%) from 79.725% when pulling a3e225f73d9fe73c001287202d6e677906ab1d65 on fix_positional_arguments into bf3a8fcda936ed13d8dd41b893ccce560c529275 on master.

myxie commented 5 months ago

@awicenec This is the work I've done to give us (optional) positional-and-keyword arguments. Would you mind providing a review?

myxie commented 5 months ago

@awicenec just a 'bump' to confirm you're happy with these changes?

awicenec commented 5 months ago

Hi Ryan,

Yes, please go ahead and merge these.

Andreas

On Thu, 6 June 2024, 03:00 Ryan Bunney, @.***> wrote:

@awicenec https://github.com/awicenec just a 'bump' to confirm you're happy with these changes?

— Reply to this email directly, view it on GitHub https://github.com/ICRAR/daliuge/pull/254#issuecomment-2151275377, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJVMSARIWKRX3LDRBHLRXTZF67ERAVCNFSM6AAAAABH7DMJZ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJRGI3TKMZXG4 . You are receiving this because you were mentioned.Message ID: @.***>