viash-io / viash

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

[BUG] `viash test` for nextflow workflows #734

Open rcannood opened 3 months ago

rcannood commented 3 months ago

What happened?

Running viash test on Nextflow workflows still doesn't work. There are two main issues:

Steps to reproduce

Run viash test on any nextflow_script (with dependencies)

Expected behavior

I expect viash test to work

Relevant log output

No response

Version

Viash 0.9.0-RC6

Possible solution

No response

Confirmation

Additional context

No response

rcannood commented 3 months ago

@Grifs I can take a look at getting viash test of a nextflow_script to work. Do you think you could take a look at getting the dependencies to be detected?

Grifs commented 3 months ago

The required functionality is 100% that of the namespace commands. After discussion we came to the conclusion that to have this functionality, it can be split up in a few chunks that are less all-encompassing. Some of these have been mentioned before as nice to have things so actually implementing it could make sense:

rcannood commented 3 months ago

The required functionality is 100% that of the namespace commands.

I think that the different between viash build|test and viash ns build|test happens at a few different levels. When compared to viash build|test, viash ns build|test:

IMO item 1 and 2 is related to user experience, but item 3 is one of the two technical things we need to fix to get viash test src/myworkflow/config.vsh.yaml to work.


Overall proposed approach

The solution that would be the easiest fix for now, is to allow viash build and viast test to resolve dependencies.

However, I agree that it's a great time to think about the user experience of the commands in general:

Add the option in namespace commands to pass a single config, e.g. viash ns test src/foo/config.vsh.yaml

I understand your point that viash build|test and viash ns build|test can be made more similar to get things to work. What we discussed a few days ago was not getting viash ns test config.vsh.yaml to work, but instead let viash test (i.e. when no config is provided) test all components in the current project. Same same but different, but I think:

is more practical than:


Verbosity

Add a verbosity option in namespace commands

  • Quiet: no extra output from components, just show the overview
  • Errors: only show error output of a failed component
  • All on Error: show all output in case a component fails
  • All: show all output of all components (owie!)
  • Auto: if only one component is being built, show "All", otherwise show "Errors"

Mostly agreed! I think the current behaviour of viash ns build|test is to show the overview in STDOUT but show errors after something has failed in STDERR. The behaviour of viash build|test is to show all output on STDOUT interactively and show an error message when something has failed in STDERR.


ns overview to STDOUT build/test logs to STDOUT build/test logs to STDERR overall status message
viash build/test / interactive / on exit
viash ns build/test interactive / on fail on exit

Is this a good summary of what the current behaviour is?

I wonder which of the proposed verbosity options sets which values in this table.


--output parameter

Add the option to build to the output structure as with non-namespace components, TBC.

Agreed. If we move viash ns build/test to viash build/test, we can:


--runner parameter

Another different between viash build and viash ns build is that the former will build just one runner (the first one), while viash ns build build all of them. (The same is also true for viash test and viash ns test, though testing is not allowed in nextflow runners so only the executable is tested).

When running viash build/test config.vsh.yaml, should we build / test all runners?

Grifs commented 3 months ago

The solution that would be the easiest fix for now, is to allow viash build and viast test to resolve dependencies.

Note that viash build|test already support resolving dependencies, the discussion started because local dependencies are not supported, right? So in that case, we should be careful with the flattening of local dependencies into a single output folder. If that is an absolute must, then they should probably be placed in the dependencies folder somehow.