-
-
Notifications
You must be signed in to change notification settings - Fork 231
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
Fix 2d output arrays with format tag #2010
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2010 +/- ##
==========================================
+ Coverage 82.87% 83.84% +0.97%
==========================================
Files 46 46
Lines 8244 8251 +7
Branches 2190 2194 +4
==========================================
+ Hits 6832 6918 +86
+ Misses 909 854 -55
+ Partials 503 479 -24 ☔ View full report in Codecov by Sentry. |
cb3f616
to
4700f30
Compare
tests/test_2D.py
Outdated
def test_output_2D_file_format() -> None: | ||
"""A simple test for format tag fix for 2D output arrays.""" | ||
|
||
Path("filename.txt").touch() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That creates a file in the current working directory. If you need to create new files, please use the tmp_path
Pytest fixture to get a safe directory to write into
https://github.com/common-workflow-language/cwltool/blob/main/tests/test_examples.py#L1263-L1268
Thank you @vjaganat90 ! I've made a note to turn your example into a new conformance test for CWL: common-workflow-language/cwl-v1.3#31 |
@mr-c if the test passes, please have another look. |
7f341eb
to
72ffef0
Compare
8b58f87
to
49a244c
Compare
Okay, the conformance test errors are due to common-workflow-language/cwltest#216 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me; and we should add a version of this as a CWL v1.3 conformance test. Any thoughts @tetron ?
Yes, I agree, and I wonder if the spec should have say anything about this behavior as well. |
Ah, yes, now that I re-read https://www.commonwl.org/v1.2/CommandLineTool.html#CommandInputParameter , it says
(The quoted text is the same for CWL v1.1 and v1.0) And yet, the example from this PR is an array of arrays of |
The issue is when we print 2d_output arrays which have "format" tag
touch filename.txt
cwltool --cachedir foo output_2D_file_format.cwl
The crux of the issue is aslist function, only promotes scalars to 1d arrays. Arbitrary JS can return an arbitrary json, format handling code needs to handle arbitrary json (not just scalars or 1d arrays which is done through aslist function).
With this fix the output should be something like this