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

cwl v1.3.0-dev1 native loop support #1779

Merged
merged 27 commits into from
Jul 17, 2024
Merged

cwl v1.3.0-dev1 native loop support #1779

merged 27 commits into from
Jul 17, 2024

Conversation

GlassOfWhiskey
Copy link
Contributor

@GlassOfWhiskey GlassOfWhiskey commented Dec 25, 2022

This commit converts the logic introduced to handle the cwltool:Loop requirement extension to handle the new loop construct introduced in the CWL v1.3 spec.
A proper method in the upgrade.py module handles the automatic conversion from the old requirement to the new format.
The existing test_loop.py file is still useful to verify the correctness of the conversion process.

See also common-workflow-language/cwl-v1.3#5

Copy link
Member

@mr-c mr-c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huzzah!

You'll also need to add a dump of the v1.3.0-dev1 schema to https://github.com/common-workflow-language/cwltool/tree/cwl-v1.3/cwltool/schemas

cwltool/update.py Outdated Show resolved Hide resolved
cwltool/update.py Outdated Show resolved Hide resolved
@mr-c mr-c changed the title Added loop support cwl v1.3.0-dev1 native loop support Dec 26, 2022
@codecov
Copy link

codecov bot commented Dec 29, 2022

Codecov Report

Attention: Patch coverage is 88.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 83.76%. Comparing base (3299069) to head (3ed2518).
Report is 28 commits behind head on main.

Files with missing lines Patch % Lines
cwltool/update.py 86.66% 0 Missing and 4 partials ⚠️
cwltool/checker.py 75.00% 0 Missing and 1 partial ⚠️
cwltool/workflow_job.py 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1779      +/-   ##
==========================================
- Coverage   83.84%   83.76%   -0.08%     
==========================================
  Files          46       46              
  Lines        8244     8262      +18     
  Branches     2190     2199       +9     
==========================================
+ Hits         6912     6921       +9     
- Misses        855      861       +6     
- Partials      477      480       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

"The `cwltool:Loop` clause is not compatible with the `when` directive."
)
if "loopWhen" not in r:
raise SourceLine(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test case for this, please

)
cls = cast(str, r["class"])
if cls in rewrite:
r["class"] = rewrite[cls]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line has no test coverage.

@tetron
Copy link
Member

tetron commented Jul 10, 2024

With the fix in common-workflow-language/schema_salad#846 this passes unit tests for me.

@mr-c
Copy link
Member

mr-c commented Jul 10, 2024

I didn't think you meant to close this PR @tetron 🙂

@tetron
Copy link
Member

tetron commented Jul 10, 2024

I didn't think you meant to close this PR @tetron 🙂

github seems to have done it automatically ☹️

@tetron
Copy link
Member

tetron commented Jul 10, 2024

Ok, unit tests passing, there's an issue with some conformance tests, and then we need to make codecov happy, but it is progress at least.

@mr-c
Copy link
Member

mr-c commented Jul 10, 2024

@tetron I think those may be due to recent changes to cwltest: common-workflow-language/cwltest#216

Did you see my email about the cofest?

@tetron
Copy link
Member

tetron commented Jul 11, 2024

Ok! So we're just down to the test coverage check.

@tetron
Copy link
Member

tetron commented Jul 16, 2024

I keep fiddling with this but I think we should just override the codecov check because at least one of the lines it is complaining about appears to be almost impossible to trigger in normal usage because it is an exception handler and "normal" exceptions get routed another way, so it only engages if a very truly unexpected case happens. Possibly we could mock something but I'm pretty sure at this point the cost/benefit of trying to make codecov happy for these corner cases is very bad.

I haven't yet managed to determine if there's a way to tell codecov to ignore certain lines.

@mr-c mr-c enabled auto-merge (squash) July 17, 2024 14:42
@mr-c mr-c disabled auto-merge July 17, 2024 17:03
@mr-c mr-c merged commit bf2b1f0 into main Jul 17, 2024
44 of 45 checks passed
@mr-c mr-c deleted the cwl-v1.3 branch July 17, 2024 17:04
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.

3 participants