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

Named ports #192

Merged
merged 18 commits into from
Aug 1, 2022
Merged

Named ports #192

merged 18 commits into from
Aug 1, 2022

Conversation

awicenec
Copy link
Contributor

Completed implementation of support for named ports. This now includes support for docker and bash components as well as the previously already supported pyfunc components. The common parts of the identification and mapping of the named ports has been moved into droputils.py. The change is backwards compatible in that it is still possible to use the command line replacement placeholders. However, there is one known side-effect which will break graphs in rare cases when they are modified using current versions of EAGLE with the named ports extension:

Mixing old and new ports: An old graph will fail, if an existing component is upgraded with a new port AND there had already been un-named ports on the same side (input or output). Work-around: All ports of that component should be connected again.

Command lines are constructed from the following parts (and in this order}:

{command} {argumentString} {command_line_arguments}

where {command} and {command_line_arguments} are defined by the respective component arguments. {argumentString} is constructed from positional and kw arguments defined as application arguments in EAGLE and augmented by ports with matching names, if defined. That means that values are read from the ports rather than from the respective argument definitions. The definition of arguments as positional is also used for port values. Arguments, which have undefined values (including zero length and white space strings and boolean False) are removed from the list, after the port matching is performed. Ports, which can't be mapped to a defined argument are not put on the command line. That feature also allows to deal with situations where the application produces outputs as a side effect (e.g. untar), but the flow needs to continue.

The implementation in principle allows mixing of a command line containing the %iX %oX placeholders, but will often lead to wrong command lines. We will replace them with new, named placeholders, which will then allow to put arguments (including ports) by name on the command line.

@awicenec awicenec requested a review from pritchardn July 30, 2022 15:53
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 82.196% when pulling 9f93047 on named-ports into a92ee81 on master.

Copy link
Collaborator

@pritchardn pritchardn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good and useful. A few points though:

  • The corner cases mentioned in the MR / how named ports work generally could probably go into the documentation too
  • Some tests for the mappings in droputils might also be good, even as a means to provide some examples.

But to get this in quickly, these things can come later.

@awicenec
Copy link
Contributor Author

awicenec commented Aug 1, 2022

Thanks Nicholas,
wrt,
Documentation: Yes, absolutely!
Tests: They are implicitly done already, since I have changed a few of the graphs, which are being run in the automatic tests. Maybe adding a few additional negative unit tests would be a good idea. We need to be able to catch quite a number of cases, which are incorrect.

@awicenec awicenec merged commit 184099b into master Aug 1, 2022
@awicenec awicenec deleted the named-ports branch August 1, 2022 06:01
awicenec added a commit that referenced this pull request Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants