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

Don't let parameter_input steps be workflow outputs #7799

Merged
merged 1 commit into from Apr 24, 2019

Conversation

@mvdbeek
Copy link
Member

@mvdbeek mvdbeek commented Apr 23, 2019

The only step types that can create workflow outputs should be
tool and subworkflow, but not data_input, data_collection_input or parameter_input
.
This fixes #7263
(and addresses some older workarounds at the root).

@jmchilton
Copy link
Member

@jmchilton jmchilton commented Apr 23, 2019

Definitely need to allow these to be outputs for CWL, added an API test that is now failing on purpose to enable this and ensure it works. Can we work around this a different way? I hadn't seen that issue but I can try to find some time to work on it.

Loading

@jmchilton
Copy link
Member

@jmchilton jmchilton commented Apr 23, 2019

xref #7016

Loading

@mvdbeek
Copy link
Member Author

@mvdbeek mvdbeek commented Apr 23, 2019

Yeah, we can just add data_input and data_collection_input to WORKFLOW_OUTPUT_STEP_TYPES , that would still fix #7262. I didn't think there would be any use for workflow inputs to be workflow outputs. Do you know what the rationale is for this requirement ?

Loading

@jmchilton
Copy link
Member

@jmchilton jmchilton commented Apr 24, 2019

I think you misunderstood me, I think parameter inputs also need to be valid workflow outputs. There just doesn't seem to me to be any good reason to exclude them. To me - doing this would be like if they designed a programming language and then explicitly excluded the identity function as a function because they didn't see the point. If you think back to the CWL example, it is built up from a spec description and it would be actually more work and verbiage in the spec to explicitly exclude something like this and anyone reading the spec I think would wonder why the authors went out of their way to prevent a certain kind of input from being an output.

We need to a reason to exclude this as an option, not a reason to enable it in my opinion - so I'm not sure there needs to be a justification for this but nonetheless I can think of good reasons to allow inputs to be outputs of workflows: perhaps you've got a script producing automated reporting built up from outputs and want to include particular inputs in that reporting for certain workflows -or- perhaps you want to build up a subworkflow iteratively and you know you will eventually want to change an input value in some way (perhaps even conditionally in the future) and so you want that to be the interface to the subworkflow but you don't want to do anything with the value at first.

All that said - I appreciate wanting to fix the bug... I'm okay with this as is as long as you let me rip it out and allow them as outputs once I have time to augment the output tracker with extra data about the source of non-data values.

Loading

@mvdbeek
Copy link
Member Author

@mvdbeek mvdbeek commented Apr 24, 2019

Yeah, I thought you meant just the data inputs. That makes sense and is more obvious for parameters than for data inputs, I see the point.

Loading

These can come in as a literal parameter value, or as a dictionary like
{'id': 'encoded_id', 'src': 'hda'}, none of which is quite right.
@mvdbeek mvdbeek force-pushed the fix_parameter_workflow_outputs branch from 5731c57 to 9f18fcf Apr 24, 2019
@mvdbeek
Copy link
Member Author

@mvdbeek mvdbeek commented Apr 24, 2019

I've reduced this to the minimal change that fixes #7262

I've also tried adding the proper tracking, but I'm not quite sure how we'd like to do this. There's some WIP in 5220723 but I'm not sure how to deal with the fact that the output to track can be a literal parameter value or a reference to a dataset

Loading

@jmchilton jmchilton merged commit 616446e into galaxyproject:dev Apr 24, 2019
6 of 7 checks passed
Loading
@jmchilton
Copy link
Member

@jmchilton jmchilton commented Apr 24, 2019

Thanks for the minimal change, I'm more comfortable with this. I'll need to find some time to take up 5220723 for the CWL branch I think so this is appreciated also.

Loading

jmchilton added a commit that referenced this issue May 24, 2019
[19.01] Backport #7799 Skip recording workflow outputs for input parameters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants