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

More defaults #68

Merged
merged 6 commits into from
Sep 2, 2021
Merged

More defaults #68

merged 6 commits into from
Sep 2, 2021

Conversation

mr-c
Copy link
Member

@mr-c mr-c commented Dec 2, 2020

No description provided.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

These go into effect for 1.2? Trying to think of consequences for existing 1.0, 1.1 implementations.

@mr-c
Copy link
Member Author

mr-c commented Dec 7, 2020

These go into effect for 1.2? Trying to think of consequences for existing 1.0, 1.1 implementations.

These were always the defaults in the text, but not in the schema as schema-salad didn't get a default field until more recently.

@mr-c
Copy link
Member Author

mr-c commented Dec 7, 2020

@kaushik-work see common-workflow-language/schema_salad#124 (comment)

To implement the slightly complicated default value rules for
ResourceRequirement
@mr-c
Copy link
Member Author

mr-c commented Dec 17, 2020

@kaushik-work on a second look, you were right, there is an issue to set these defaults there

Looking at the text for https://www.commonwl.org/v1.2/CommandLineTool.html#ResourceRequirement

Specify basic hardware resource requirements.

"min" is the minimum amount of a resource that must be reserved to schedule a job. If "min" cannot be satisfied, the job should not be run.

Later in the table we are told there are default values for coreMin (1), ramMin (256), tmpdirMin (1024), and outdirMin (1024),

If both "min" and "max" are specified, an implementation may choose to allocate any amount between "min" and "max", with the actual allocation provided in the runtime object.

If I add those defaults values to the schema as I do in this PR, then strictly speaking all the *Min fields will always be specified, and this sentence is meaningless.

If "min" is specified but "max" is not, then "max" == "min" If "max" is specified by "min" is not, then "min" == "max".

Same here. The "min"s will always be specified, so this rule simplifies down to If "max" is not specified, then "max" == "min"

If neither "min" nor "max" is specified for a resource, use the default values below.

So these defaults have to be applied at a different layer than the schema to enable this fallback processing.

Therefore I have updated this PR to add notes to prevent someone else from making the same mistake.

I also added two legitimately missing defaults: InputBinding.position default is 0, and File.streamable default is false.

@mr-c mr-c requested a review from a user December 17, 2020 09:00
@mr-c mr-c enabled auto-merge (squash) January 19, 2021 17:55
@mr-c mr-c changed the base branch from main to 1.2.1_proposed September 2, 2021 15:23
@mr-c mr-c merged commit cfeb421 into 1.2.1_proposed Sep 2, 2021
@mr-c mr-c deleted the more_defaults branch September 2, 2021 15:23
mr-c added a commit that referenced this pull request Oct 9, 2021
* some defaults should not be set using 'default'

To implement the slightly complicated default value rules for
ResourceRequirement

* InputBinding.position default is 0.

* File.streamable default is false.
kinow pushed a commit to kinow/cwl-v1.2 that referenced this pull request Oct 20, 2022
* some defaults should not be set using 'default'

To implement the slightly complicated default value rules for
ResourceRequirement

* InputBinding.position default is 0.

* File.streamable default is false.
GlassOfWhiskey pushed a commit that referenced this pull request Nov 15, 2023
* some defaults should not be set using 'default'

To implement the slightly complicated default value rules for
ResourceRequirement

* InputBinding.position default is 0.

* File.streamable default is false.
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.

1 participant