viash-io / viash

script + metadata = standalone component
https://viash.io
GNU General Public License v3.0
39 stars 2 forks source link

split up arguments into inputs and outputs #55

Closed rcannood closed 2 years ago

rcannood commented 3 years ago

Current:

functionality:
  arguments:
    - type: file
      direction: input
      name: "--input"
    - type: file
      direction: output
      name: "--output"
    - type: string
      name: "--str"
      default: foo

Proposal:

functionality:
  inputs:
    - name: "--input"
  outputs:
    - name: "--output"
  arguments:
    - type: string
      name: "--str"
      default: foo

Edit: updated this description based on Toni's proposal at the bottom.

tverbeiren commented 3 years ago

In principle those are two different representations of the same thing. However, there is a difference: the array of arguments in the current implementation has an order to it.

If we simply wrap a binary (no custom script) and the underlying tool requires positional arguments the proposal does not contain the order anymore. The current version does.

BlackDragonBE commented 3 years ago

One way of mitigating the argument order issue is adding an optional "order" int attribute . The argument order for the final component could then use those to sort the array.
However, that does complicate the setup of the config file.

rcannood commented 3 years ago

Hmm. If order is an issue then keeping the config the way its currently done makes more sense.

Does wrapping a binary without script still work? Is it something we want to continue supporting in the future?

rcannood commented 3 years ago

In talking it over with @tverbeiren, it might be a good idea to keep the arguments ordered, because we don't know whether we might need it in the future.

Adding an order: 1 to each argument rather than needing to specify the direction the argument uses, might reduce the amount of typing that needs to be done in 99.9% of viash components. However, I find it less intuitive than simply using 'arguments'.

Here's another alternative! We support both 'inputs/outputs' and 'arguments'. Internally, viash will create and use the following list:

case class Functionality(..., inputs, outputs, arguments, ...) {
  val internal_args = arguments +++
    inputs.map(_.copy(direction = Input)) +++ 
    outputs.map(_.copy(direction = Output)
}

most people can then use inputs and outputs. If order is important, you can use arguments.

@BlackDragonBE @tverbeiren Thoughts?

tverbeiren commented 3 years ago

Alternative suggestion: We keep arguments and allow for two different representations of arguments of the type {type: file, direction: input/output}. The following two would be identical (up to the implicit order, that is):

functionality:
  arguments:
    - type: file
      direction: input
      name: "--input"
    - type: file
      direction: output
      name: "--output"
    - type: string
      name: "--modifier"

With explicit inputs and outputs, implicit type: file for those and probably also required: true:

functionality:
  inputs:
    - name: "--input"
  outputs:
    - name: "--output"
  arguments:
    - type: string
      name: "--modifier"
rcannood commented 3 years ago

Hmm. If we plan on allowing json inputs and outputs, I'm not sure implicitly setting the type to file would be a good idea.

Otherwise, I'm in favour! Let me think about it :)

BlackDragonBE commented 3 years ago

I like Tony's proposal as well, and I agree to keep the type declaration intact in both scenarios. 👍

rcannood commented 2 years ago

This functionality is included in Viash 0.5.11! Thanks @Grifs !

rcannood commented 2 years ago

See https://github.com/viash-io/viash/pull/120/files#diff-9f3b89b8a79a1a7ea41ce883a007ffab690670c3eb1f6cb606d2fe8a47a4de5fR38-R56 for implementation.

Arguments have, by default, required=false. Should inputs and outputs have required=true by default?

There is already a change for the inputs and outputs, namely:

I'm considering also overriding the required value, so:

Which allows you to write:

functionality:
  name: foo
  inputs: { name: --input }
  outputs: { name: --output }
  arguments:
    - { type: string, name: --arg, default: foo }
    - { type: integer, name: --int, default: 123 }
    - { type: double, name: --dou, default: 0.5 }

@Grifs @tverbeiren What do you think?

tverbeiren commented 2 years ago

Not in my opinion, I would keep this aligned for all arguments.

Grifs commented 2 years ago

I agree with Toni's opinion, besides, it would be a perfect setting to set in a global config file ;)