Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support ArrayNode mapping over Launch Plans #2480
Support ArrayNode mapping over Launch Plans #2480
Changes from 23 commits
62031c3
2293b6a
9579da6
683b50a
11d0dca
4155c68
99322e9
2b051c3
2c6991e
80f7140
8faf457
52bae8f
f0bc6dc
5e8cd9d
7ff17b0
91a6438
d5b32d2
ced84ba
45cc9ac
d14f97e
7315ab5
e56e57c
f12c7cc
c3f6fe7
8ae1ae2
7fef6d0
34210cf
21d286a
5b7fbcc
c59c427
8340c08
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Reformat this docstring to use the reST format (e.g. [1] or [2]).
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.
why do we need to special case
min_successes
?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.
Since we default min_success_ratio to 1.0, it's always set. When passing in both into the proto the one_of will take the last assigned value - which would be min_success_ratio if both are set.
Could also handle this by nulling out min_success_ratio if min_successes is set
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.
what do you think of the idea of changing the type of
min_success_ratio
inmap_task
? Fromfloat
toOptional[float]
and set its default value toNone
but at the same time check if bothmin_successes
andmin_success_ratio
are set toNone
we "favor"min_success_ratio
and set that to1.0
?This way we centralize this weird interaction between two different parameters in a single function and we don't really validate anything in
ArrayNodeModel
(so that instantiating objects of that type will always take the arguments as-is) and no more special-casing.