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

Added loop support #5

Merged
merged 9 commits into from
Oct 7, 2024
Merged

Added loop support #5

merged 9 commits into from
Oct 7, 2024

Conversation

GlassOfWhiskey
Copy link
Collaborator

This commit adds the previous Loop requirement extension directly into the CWL v1.3 spec.

@GlassOfWhiskey
Copy link
Collaborator Author

The important question here is: do we still need loopSource or we can directly use the source field here?
I remember the problem was that the source field requires a step_name/output_name format for the argument, while the loopSource here requires just the output_name, as the step_name is always the current one.

I think that we can proceed in two ways: either modify the schema_salad to handle both behaviors, or keep the loopSource specific definition.

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.

Needs loop conformance tests, and needs all the tests to use cwlVersion: v1.3.0-dev1 everywhere except for the mixed versions tests which should be adapted with care.

For testing purposes you could also adjust the CI to use cwltool from your PR. We can revert that bit once there is a cwltool release with v1.3.0-dev1 support.

Workflow.yml Outdated Show resolved Hide resolved
@GlassOfWhiskey GlassOfWhiskey force-pushed the loop-integration branch 5 times, most recently from e62e5b3 to e99c631 Compare December 26, 2022 17:10
@GlassOfWhiskey
Copy link
Collaborator Author

GlassOfWhiskey commented Dec 28, 2022

Putting this PR in Draft state, as it depends on this one.

@GlassOfWhiskey GlassOfWhiskey marked this pull request as draft December 28, 2022 20:56
@GlassOfWhiskey GlassOfWhiskey force-pushed the loop-integration branch 2 times, most recently from c31c3ff to 4e5d223 Compare January 3, 2023 16:39
@GlassOfWhiskey GlassOfWhiskey marked this pull request as ready for review January 3, 2023 17:28
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.

Needs a changelog entry and ideally some tests that don't use InlineJavascriptRequirement

CODE_OF_CONDUCT.md Outdated Show resolved Hide resolved
@GlassOfWhiskey GlassOfWhiskey force-pushed the loop-integration branch 2 times, most recently from a0913f2 to 145185f Compare May 16, 2024 13:43
@GlassOfWhiskey GlassOfWhiskey force-pushed the loop-integration branch 2 times, most recently from e1433bd to 6670c07 Compare May 16, 2024 13:55
@mr-c
Copy link
Member

mr-c commented May 16, 2024

Needs a changelog entry

This is still true

Workflow.yml Outdated Show resolved Hide resolved
Workflow.yml Outdated Show resolved Hide resolved
Workflow.yml Outdated Show resolved Hide resolved
Workflow.yml Outdated Show resolved Hide resolved
Workflow.yml Outdated Show resolved Hide resolved
Workflow.yml Outdated Show resolved Hide resolved
Workflow.yml Outdated Show resolved Hide resolved
@tom-tan
Copy link
Member

tom-tan commented May 17, 2024

I have a simple question about loop feature.

Is it included as mandatory, or optional (e.g., do we need the LoopRequirement in the requirements field)?

I understand it is essentially needed for several applications.
However, IMO, it would be nice if it is defined as an optional feature with the following reasons:

  • There exists many applications that do not need loops. Reqiring all the workflow engines to be implemented a loop feature needs too much effort.
  • Making loops as optional makes easier to develop CWL compliant engines than making it as mandatory. By keeping the mandatory spec smaller and simple as possible, we can provide users with more choices for workflow engines.

@mr-c
Copy link
Member

mr-c commented May 22, 2024

@tom-tan We could make it optional but without needing a LoopRequirement, like conditionals are in CWL v1.2: https://www.commonwl.org/v1.2/Workflow.html#Conditional_execution_(Optional)

@tetron
Copy link
Member

tetron commented Jul 18, 2024

My comment about "Could we expand the names to last_iteration_output and all_iteration_outputs" still applies, the rest looks good.

- name: LoopOutputMethod
type: enum
docParent: "#LoopWorkflowStep"
doc: The loop output method, as described in [workflow step loop](#LoopWorkflowStep).
Copy link
Member

@mr-c mr-c Jul 18, 2024

Choose a reason for hiding this comment

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

please, also described here :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This format is compliant to the scatterMetod field documented just above

@mr-c
Copy link
Member

mr-c commented Jul 19, 2024

There was an error in how we were running the self-check (are all the CWL documents used in the conformance tests valid at the schema level) which I fixed in #46 and rebased into here

tests/loop/invalid-no-when.cwl is invalid at the CWL schema level on purpose, which causes that check to fail. I'm not against having it in the conformance tests, but we need a way to easily mark it so we don't do the schema validation on it.

  1. Filename pattern; Alas invalid is also used in other conformance tests that are schema-salad compliant. Either we rename those files or find another word to stick in the CWL document filename
  2. put schema-breaking tests in tests/invalid folder or similar

@GlassOfWhiskey
Copy link
Collaborator Author

I think the best idea would be to simply remove the test, as we can easily rely on SchemaSalad validation to assess this behaviour, without the need for a specific test

@@ -24,7 +24,7 @@ jobs:
python-version: '3.9.x'

- name: Setup prerequirements
run: pip install "cwltest>=2.3" pytest-xdist "git+https://github.com/common-workflow-language/cwltool.git@main#egg=cwltool"
run: pip install "cwltest>=2.3" pytest-xdist "cwltool>=3.1.20240909164951"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
run: pip install "cwltest>=2.3" pytest-xdist "cwltool>=3.1.20240909164951"
run: pip install "cwltest>=2.3" pytest-xdist "cwltool>=3.1.20241007082533"

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.

Please add a changelog, otherwise I approve!

@mr-c mr-c merged commit 26f8861 into main Oct 7, 2024
7 of 8 checks passed
@mr-c mr-c deleted the loop-integration branch October 7, 2024 16:35
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.

4 participants