-
Notifications
You must be signed in to change notification settings - Fork 359
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
[WX-1468] Implement returnCodes
runtime attribute
#7389
Conversation
backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala
Outdated
Show resolved
Hide resolved
This is fantastic work! Which makes me feel terrible saying: I hadn't realized until reviewing the spec just now that the new |
I abstracted out the duplicated code between |
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 is a big improvement, but I think we can go even farther in collapsing these two concepts.
* `continueOnReturnCode` will be used. | ||
*/ | ||
lazy val returnCode: ReturnCode = | ||
if (returnCodes.isInstanceOf[ReturnCodeSet] && returnCodes.equals(ReturnCodeSet(Set(0)))) continueOnReturnCode |
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.
It looks like this is checking whether we have the default value for returnCodes
and falling back to continueOnReturnCode
if we do. What if someone included the default value in their WDL? Can we insert the default after we choose which attribute to use?
If not, we could build the default check into ReturnCode
, so we can call something like returnCodes.isDefault
?
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.
Also, just for fun, a more idiomatic Scala way to write this is:
returnCodes match {
case ReturnCodeSet(Set(0)) => continueOnReturnCode
case _ => returnCodes
}
* @param returnCode Return code from the process / script. | ||
* @return True if the call is a success. | ||
*/ | ||
override def continueFor(returnCode: Int): Boolean = super.continueFor(returnCode) |
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 method looks like a no-op, since it overrides the super
method and then calls it.
instance.withDefault(configDefaultWdlValue(runtimeConfig) getOrElse WomInteger(0)) | ||
def configDefaultWdlValue(runtimeConfig: Option[Config]): Option[WomValue] = | ||
instance.configDefaultWomValue(runtimeConfig) | ||
} | ||
|
||
class ContinueOnReturnCodeValidation extends RuntimeAttributesValidation[ContinueOnReturnCode] { | ||
class ContinueOnReturnCodeValidation extends ReturnCodeValidation { |
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.
I would love to see if we can collapse all the return code stuff into this validation, and have this be the last thing in the code path that's aware of the two different attributes. We could even continue calling it ContinueOnReturnCode
to minimize the diff. This validation can look at both keys and make a decision about what ContinueOnReturnCode
object to build. We can turn the *
into a ContinueOnReturnCodeFlag(true)
, which seems to have the same meaning.
I believe that all of the comments here should be addressed in my most recent refactor which groups |
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.
Love that this is mostly test changes at this point! Very nice!
@@ -96,6 +96,14 @@ jobs: | |||
- uses: ./.github/set_up_cromwell_action #This github action will set up git-secrets, caching, java, and sbt. | |||
with: | |||
cromwell_repo_token: ${{ secrets.BROADBOT_GITHUB_TOKEN }} | |||
# If a build step fails, activate SSH and idle for 30 minutes. | |||
- name: Setup tmate session |
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.
Should we take this out before merging, or is it meant to be permanent?
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.
Yes, this will be removed before merging (its still in for debugging right now)
backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala
Outdated
Show resolved
Hide resolved
backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala
Outdated
Show resolved
Hide resolved
backend/src/main/scala/cromwell/backend/validation/TwoKeyRuntimeAttributesValidation.scala
Outdated
Show resolved
Hide resolved
backend/src/main/scala/cromwell/backend/validation/ValidatedRuntimeAttributesBuilder.scala
Outdated
Show resolved
Hide resolved
backend/src/test/scala/cromwell/backend/validation/RuntimeAttributesValidationSpec.scala
Outdated
Show resolved
Hide resolved
centaur/src/main/resources/standardTestCases/failing_return_code.test
Outdated
Show resolved
Hide resolved
backend/src/main/scala/cromwell/backend/validation/ContinueOnReturnCodeValidation.scala
Outdated
Show resolved
Hide resolved
...eturn_codes_and_continue_on_return_code/invalid_return_codes_and_continue_on_return_code.wdl
Outdated
Show resolved
Hide resolved
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.
I really like the new direction. Always worth it to put in extra effort to get less code.
I do have one comment about reducing duplication (ironic, I know, in light of the biscayne
vs. cascades
thing...) but this looks good and very close!
wdl/transforms/cascades/src/main/scala/wdl/transforms/cascades/wdlom2wom/package.scala
Show resolved
Hide resolved
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 is shaping up really well! I think getFinalRuntimeAttributes
could use a pass of Scala-ification, I left a few comments but probably a good topic for mob time tomorrow.
Implemented the returnCodes runtime attribute, which is new in WDL 1.1.