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

--write-summary to redirect final JSON output #1773

Merged
merged 5 commits into from
Dec 13, 2022

Conversation

jfennick
Copy link
Contributor

@jfennick jfennick commented Dec 3, 2022

The goal of this PR is to allow users to hide the final output json. For large workflows (particularly workflows with scattering) this can be absolutely massive. If the workflow completes successfully this isn't necessarily a problem. However, if the workflow fails, users will need to scroll up past the final output so they can see the error message(s).

The problem I'm finding is that (despite my urging) users are apparently unable or unwilling to scroll up...

The solution I've chosen here simply adds a CLI flag --no-print-final-output which hides the final output. Note that using --provenance, this information is still available at provenance/workflow/final-output.json

Another solution is instead of using print, the code could use the python logging API. Then developers using the python API could use a logging filter to hide the final output. However, this would not help CLI users.

Another solution is to neither print nor log the final output, but instead to write it directly to a file. However, as noted above, this basically already exists at provenance/workflow/final-output.json

@codecov
Copy link

codecov bot commented Dec 3, 2022

Codecov Report

Merging #1773 (a5d63ee) into main (38f42e6) will decrease coverage by 0.01%.
The diff coverage is 90.47%.

@@            Coverage Diff             @@
##             main    #1773      +/-   ##
==========================================
- Coverage   82.51%   82.50%   -0.02%     
==========================================
  Files          47       47              
  Lines        8197     8203       +6     
  Branches     2235     2237       +2     
==========================================
+ Hits         6764     6768       +4     
- Misses        939      941       +2     
  Partials      494      494              
Impacted Files Coverage Δ
cwltool/main.py 75.81% <89.47%> (+0.15%) ⬆️
cwltool/argparser.py 95.23% <100.00%> (+0.01%) ⬆️
cwltool/cwlrdf.py 29.06% <100.00%> (ø)
cwltool/job.py 81.21% <0.00%> (-0.40%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jfennick
Copy link
Contributor Author

jfennick commented Dec 3, 2022

I'm not sure how to test differential code coverage locally so codecov/patch will pass. Any ideas?

@mr-c mr-c force-pushed the no_print_final_output branch from ad4762a to 6aeb924 Compare December 7, 2022 07:07
@mr-c
Copy link
Member

mr-c commented Dec 7, 2022

Another solution is to neither print nor log the final output, but instead to write it directly to a file. However, as noted above, this basically already exists at provenance/workflow/final-output.json

Hey @jfennick , thanks for sharing. There is a lot of important information in the CWL output object (which is the only thing written to /dev/stdout). For example, non-File outputs are only represented in this JSON dump.

So I don't think it would be safe to include an option that can lead to dataloss.

The could be a --provenance-hide-final-output option that also requires the use of --provenance. That would be safe since, as you pointed out, the final output object is saved in the RO Bundle.

And/or there could be an option to save the final output object to a particular path. This is equivalent to redirecting the stdout stream at the command line (cwltool my-workflow.cwl my_inputs.yaml > my_workflow_outputs.json) but by having an explicit option the help text will remind the user of this choice.

Perhaps that other option could be named --output-write or similar with -o as an alias.

@jfennick
Copy link
Contributor Author

jfennick commented Dec 8, 2022

Gotcha, I didn't notice the difference w.r.t. non-File outputs. I think the easiest / safest choice is your last option, --output-write. I'll get started on that soon. We can perhaps add a --provenance-hide-final-output option later.

FYI the output object is indeed the only thing printed to stdout, if you use the --quiet flag. But if not, the docker commands are printed, and more importantly the stdout of each Process is also printed, which may contain useful information that users may not find in the respective log files, so for now I would prefer to keep printing that information and just selectively hide the final output object.

@jfennick jfennick changed the title added --no-print-final-output added --output-write Dec 8, 2022
@jfennick
Copy link
Contributor Author

jfennick commented Dec 8, 2022

Now codecov/patch is passing and codecov/project is failing. ??

@jfennick
Copy link
Contributor Author

Any chance we can get this merged before the holidays? I'm not sure what else I need to do to get codecov/project to pass.

tests/test_iwdr.py Outdated Show resolved Hide resolved
@mr-c mr-c changed the title added --output-write --write-summary to redirect final JSON output Dec 13, 2022
@mr-c mr-c enabled auto-merge (squash) December 13, 2022 11:04
@mr-c mr-c force-pushed the no_print_final_output branch 2 times, most recently from ef6edf9 to 432bc94 Compare December 13, 2022 12:27
@mr-c mr-c force-pushed the no_print_final_output branch from 432bc94 to a5d63ee Compare December 13, 2022 13:19
@mr-c mr-c merged commit 6a10aa5 into common-workflow-language:main Dec 13, 2022
@jfennick
Copy link
Contributor Author

Excellent, thank you!

@jfennick jfennick deleted the no_print_final_output branch March 9, 2023 04:45
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.

2 participants