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

[WX-1468] Implement returnCodes runtime attribute #7389

Merged
merged 28 commits into from
Apr 11, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .github/workflows/integration_tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

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?

Copy link
Contributor Author

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)

if: ${{ failure() }}
uses: mxschmitt/action-tmate@v3
timeout-minutes: 90
with:
limit-access-to-actor: true
detached: true
#This script bascially just looks up another script to run, assuming that the other script's filename is:
#src/ci/bin/test${BUILD_TYPE}.sh. The first letter of the BUILD_TYPE is automatically capitalized when looking.
- name: Run Integration Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import org.apache.commons.lang3.StringUtils
import org.apache.commons.lang3.exception.ExceptionUtils
import shapeless.Coproduct
import wom.RuntimeAttributesKeys.ReturnCodesKey
import wom.callable.{AdHocValue, CommandTaskDefinition, ContainerizedInputExpression}
import wom.expression.WomExpression
import wom.graph.LocalName
Expand Down Expand Up @@ -748,13 +749,31 @@
RuntimeAttributesValidation.extract(FailOnStderrValidation.instance, validatedRuntimeAttributes)

/**
* Returns the behavior for continuing on the return code, obtained by converting `returnCodeContents` to an Int.
*
* @return the behavior for continuing on the return code.
*/
* Returns the behavior for continuing on the return code, obtained by converting `returnCodeContents` to an Int.
*
* @return the behavior for continuing on the return code.
*/
lazy val continueOnReturnCode: ContinueOnReturnCode =
RuntimeAttributesValidation.extract(ContinueOnReturnCodeValidation.instance, validatedRuntimeAttributes)

/**
* Returns the behavior for continuing on the return code, obtained by converting `returnCodeContents` to an Int.
*
* @return the behavior for continuing on the return code.
*/
lazy val returnCodes: ContinueOnReturnCode =
RuntimeAttributesValidation.extract[ContinueOnReturnCode](ReturnCodesKey, validatedRuntimeAttributes)

/**
* Returns the true behavior for continuing on the return code. If `returnCodes` runtime attribute is specified,
* then `continueOnReturnCode` attribute is ignored. If `returnCodes` is unspecified, then the value in
* `continueOnReturnCode` will be used.
*/
lazy val returnCode: ContinueOnReturnCode =
if (returnCodes.isInstanceOf[ContinueOnReturnCodeSet] && returnCodes.equals(ContinueOnReturnCodeSet(Set(0))))
continueOnReturnCode
else returnCodes
rsaperst marked this conversation as resolved.
Show resolved Hide resolved

/**
* Returns the max number of times that a failed job should be retried, obtained by converting `maxRetries` to an Int.
*/
Expand Down Expand Up @@ -1388,7 +1407,7 @@
)
)
retryElseFail(executionHandle)
case Success(returnCodeAsInt) if continueOnReturnCode.continueFor(returnCodeAsInt) =>
case Success(returnCodeAsInt) if returnCode.continueFor(returnCodeAsInt) =>
handleExecutionSuccess(status, oldHandle, returnCodeAsInt)
// It's important that we check retryWithMoreMemory case before isAbort. RC could be 137 in either case;
// if it was caused by OOM killer, want to handle as OOM and not job abort.
Expand Down Expand Up @@ -1422,7 +1441,7 @@
} else {
tryReturnCodeAsInt match {
case Success(returnCodeAsInt)
if outOfMemoryDetected && memoryRetryRequested && !continueOnReturnCode.continueFor(returnCodeAsInt) =>
if outOfMemoryDetected && memoryRetryRequested && !returnCode.continueFor(returnCodeAsInt) =>

Check warning on line 1444 in backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala

View check run for this annotation

Codecov / codecov/patch

backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala#L1444

Added line #L1444 was not covered by tests
rsaperst marked this conversation as resolved.
Show resolved Hide resolved
val executionHandle = Future.successful(
FailedNonRetryableExecutionHandle(
RetryWithMoreMemory(jobDescriptor.key.tag, stderrAsOption, memoryRetryErrorKeys, log),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
import cats.data.Validated.{Invalid, Valid}
import cats.implicits._
import com.typesafe.config.Config
import cromwell.backend.validation.RuntimeAttributesValidation._
import common.validation.ErrorOr._
import cromwell.backend.validation.RuntimeAttributesValidation.validateInt
import wom.RuntimeAttributesKeys
import wom.types._
import wom.values._
Expand All @@ -23,22 +23,31 @@
* `default` a validation with the default value specified by the reference.conf file.
*/
object ContinueOnReturnCodeValidation {
lazy val instance: RuntimeAttributesValidation[ContinueOnReturnCode] = new ContinueOnReturnCodeValidation
def default(runtimeConfig: Option[Config]): RuntimeAttributesValidation[ContinueOnReturnCode] =
instance.withDefault(configDefaultWdlValue(runtimeConfig) getOrElse WomInteger(0))
lazy val instance: TwoKeyRuntimeAttributesValidation[ContinueOnReturnCode, ContinueOnReturnCodeSet] =
new ContinueOnReturnCodeValidation
def default(
runtimeConfig: Option[Config]
): TwoKeyRuntimeAttributesValidation[ContinueOnReturnCode, ContinueOnReturnCodeSet] =
instance.makeDefault(configDefaultWdlValue(runtimeConfig) getOrElse WomInteger(0))
def configDefaultWdlValue(runtimeConfig: Option[Config]): Option[WomValue] =
instance.configDefaultWomValue(runtimeConfig)
instance.configDefault(runtimeConfig)
}

class ContinueOnReturnCodeValidation extends RuntimeAttributesValidation[ContinueOnReturnCode] {
class ContinueOnReturnCodeValidation
extends TwoKeyRuntimeAttributesValidation[ContinueOnReturnCode, ContinueOnReturnCodeSet] {

override def key: String = RuntimeAttributesKeys.ContinueOnReturnCodeKey
override def key: String = RuntimeAttributesKeys.ReturnCodesKey

override def altKey: String = RuntimeAttributesKeys.ContinueOnReturnCodeKey

override def defaultVal: ContinueOnReturnCodeSet = ContinueOnReturnCodeSet(Set(0))

override def coercion: Set[WomType] = ContinueOnReturnCode.validWdlTypes

override def validateValue: PartialFunction[WomValue, ErrorOr[ContinueOnReturnCode]] = {
case WomBoolean(value) => ContinueOnReturnCodeFlag(value).validNel
case WomString(value) if Try(value.toBoolean).isSuccess => ContinueOnReturnCodeFlag(value.toBoolean).validNel
case WomString(value) if value.equals("*") => ContinueOnReturnCodeFlag(true).validNel

Check warning on line 50 in backend/src/main/scala/cromwell/backend/validation/ContinueOnReturnCodeValidation.scala

View check run for this annotation

Codecov / codecov/patch

backend/src/main/scala/cromwell/backend/validation/ContinueOnReturnCodeValidation.scala#L50

Added line #L50 was not covered by tests
case WomString(value) if Try(value.toInt).isSuccess => ContinueOnReturnCodeSet(Set(value.toInt)).validNel
case WomInteger(value) => ContinueOnReturnCodeSet(Set(value)).validNel
case value @ WomArray(_, seq) =>
Expand All @@ -47,18 +56,21 @@
case Valid(ints) => ContinueOnReturnCodeSet(ints.toSet).validNel
case Invalid(_) => invalidValueFailure(value)
}
case value => invalidValueFailure(value)

Check warning on line 59 in backend/src/main/scala/cromwell/backend/validation/ContinueOnReturnCodeValidation.scala

View check run for this annotation

Codecov / codecov/patch

backend/src/main/scala/cromwell/backend/validation/ContinueOnReturnCodeValidation.scala#L59

Added line #L59 was not covered by tests
}

override def validateExpression: PartialFunction[WomValue, Boolean] = {
case WomBoolean(_) => true
case WomString(value) if Try(value.toInt).isSuccess => true
case WomString(value) if Try(value.toBoolean).isSuccess => true
case WomString(value) if value.equals("*") => true
case WomString(value) if Try(value.toInt).isSuccess => true

Check warning on line 66 in backend/src/main/scala/cromwell/backend/validation/ContinueOnReturnCodeValidation.scala

View check run for this annotation

Codecov / codecov/patch

backend/src/main/scala/cromwell/backend/validation/ContinueOnReturnCodeValidation.scala#L65-L66

Added lines #L65 - L66 were not covered by tests
case WomInteger(_) => true
case WomArray(WomArrayType(WomStringType), elements) =>
elements forall { value =>
Try(value.valueString.toInt).isSuccess
}
case WomArray(WomArrayType(WomIntegerType), _) => true
case _ => false

Check warning on line 73 in backend/src/main/scala/cromwell/backend/validation/ContinueOnReturnCodeValidation.scala

View check run for this annotation

Codecov / codecov/patch

backend/src/main/scala/cromwell/backend/validation/ContinueOnReturnCodeValidation.scala#L73

Added line #L73 was not covered by tests
}

override protected def missingValueMessage: String = s"Expecting $key" +
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
package cromwell.backend.validation

import com.typesafe.config.Config
import common.validation.ErrorOr.ErrorOr
import cromwell.backend.RuntimeAttributeDefinition
import cromwell.backend.validation.RuntimeAttributesValidation.extractOption
import wom.types.WomType
import wom.values.{WomString, WomValue}

object TwoKeyRuntimeAttributesValidation {
def withDefault[ValidatedType, DefaultValType](
validation: TwoKeyRuntimeAttributesValidation[ValidatedType, DefaultValType],
default: WomValue
): TwoKeyRuntimeAttributesValidation[ValidatedType, DefaultValType] =
new TwoKeyRuntimeAttributesValidation[ValidatedType, DefaultValType] {
override def key: String = validation.key

override def altKey: String = validation.altKey

override def defaultVal: DefaultValType = validation.defaultVal

override def coercion: Iterable[WomType] = validation.coercion

override protected def validateValue: PartialFunction[WomValue, ErrorOr[ValidatedType]] =
validation.validateValuePackagePrivate

override protected def validateExpression: PartialFunction[WomValue, Boolean] =
validation.validateExpressionPackagePrivate

override protected def invalidValueMessage(value: WomValue): String =
validation.invalidValueMessagePackagePrivate(value)

Check warning on line 31 in backend/src/main/scala/cromwell/backend/validation/TwoKeyRuntimeAttributesValidation.scala

View check run for this annotation

Codecov / codecov/patch

backend/src/main/scala/cromwell/backend/validation/TwoKeyRuntimeAttributesValidation.scala#L31

Added line #L31 was not covered by tests

override protected def missingValueMessage: String = s"Expecting $key runtime attribute to be a type in $coercion"

override def usedInCallCaching: Boolean = validation.usedInCallCachingPackagePrivate

override protected def staticDefaultOption = Option(default)
}

/**
* Returns the value from the attributes matching one of the validation keys. If values are assigned to attribute
* matching both keys, the `key` field will override the `altKey` field on the `RuntimeAttributeValidation`.
*
* Do not use an optional validation as the type internal implementation will throw a `ClassCastException` due to the
* way values are located and auto-magically cast to the type of the `runtimeAttributesValidation`.
*
* @param runtimeAttributesValidation The typed validation to use.
* @param validatedRuntimeAttributes The values to search.
* @return The value matching the key.
* @throws ClassCastException if the validation is called on an optional validation.
*/
def extractTwoKeys[A, B](runtimeAttributesValidation: TwoKeyRuntimeAttributesValidation[A, B],
validatedRuntimeAttributes: ValidatedRuntimeAttributes
): A = {
val key = runtimeAttributesValidation.key
val altKey = runtimeAttributesValidation.altKey
val primaryValue = extractOption(key, validatedRuntimeAttributes)
val altValue = extractOption(altKey, validatedRuntimeAttributes)
val value =
if (
primaryValue.isEmpty || (primaryValue.contains(
runtimeAttributesValidation.defaultVal
) && !altValue.isEmpty && !altValue.contains(
runtimeAttributesValidation.defaultVal
))
) altValue

Check warning on line 66 in backend/src/main/scala/cromwell/backend/validation/TwoKeyRuntimeAttributesValidation.scala

View check run for this annotation

Codecov / codecov/patch

backend/src/main/scala/cromwell/backend/validation/TwoKeyRuntimeAttributesValidation.scala#L66

Added line #L66 was not covered by tests
else primaryValue
value match {
// NOTE: Some(innerValue) aka Some.unapply() throws a `ClassCastException` to `Nothing$` as it can't tell the type
case some: Some[_] => some.get.asInstanceOf[A]
case None =>
throw new RuntimeException(

Check warning on line 72 in backend/src/main/scala/cromwell/backend/validation/TwoKeyRuntimeAttributesValidation.scala

View check run for this annotation

Codecov / codecov/patch

backend/src/main/scala/cromwell/backend/validation/TwoKeyRuntimeAttributesValidation.scala#L72

Added line #L72 was not covered by tests
s"$key not found in runtime attributes ${validatedRuntimeAttributes.attributes.keys}"
)
}
}
}

trait TwoKeyRuntimeAttributesValidation[A, DefaultValType] extends RuntimeAttributesValidation[A] {
rsaperst marked this conversation as resolved.
Show resolved Hide resolved

/**
* Returns the alternate key of the runtime attribute.
*
* @return the alternate key of the runtime attribute.
*/
def altKey: String

/**
* The default value of this runtime attribute.
*
* @return the default value of this runtime attribute.
*/
def defaultVal: DefaultValType

/**
* Runs this validation on the value matching key.
*
* NOTE: The values passed to this method should already be evaluated instances of WomValue, and not WomExpression.
*
* @param values The full set of values.
* @return The error or valid value for this key.
*/
def validateAltKey(values: Map[String, WomValue]): ErrorOr[A] =
values.get(altKey) match {
case Some(value) => validateValue.applyOrElse(value, (_: Any) => invalidValueFailure(value))
case None => validateNone

Check warning on line 106 in backend/src/main/scala/cromwell/backend/validation/TwoKeyRuntimeAttributesValidation.scala

View check run for this annotation

Codecov / codecov/patch

backend/src/main/scala/cromwell/backend/validation/TwoKeyRuntimeAttributesValidation.scala#L106

Added line #L106 was not covered by tests
}

/**
* Returns a version of this validation with the default value.
*
* @param womValue The default wdl value.
* @return The new version of this validation.
*/
final def makeDefault(womValue: WomValue): TwoKeyRuntimeAttributesValidation[A, DefaultValType] =
TwoKeyRuntimeAttributesValidation.withDefault(this, womValue)

/**
* Returns the value of the default runtime attribute of a
* validation key as specified in the reference.conf. Given
* a value, this method coerces it into an optional
* WomValue. In case the value cannot be succesfully coerced
* the value is wrapped as a "BadDefaultAttributeValue" type that
* is failed downstream by the ValidatedRuntimeAttributesBuilder.
*
* @param optionalRuntimeConfig Optional default runtime attributes config of a particular backend.
* @return The new version of this validation.
*/
final def configDefault(optionalRuntimeConfig: Option[Config]): Option[WomValue] =
optionalRuntimeConfig collect {
case config if config.hasPath(key) || config.hasPath(altKey) =>
val value = if (config.hasPath(key)) config.getValue(key).unwrapped() else config.getValue(altKey).unwrapped()
coercion collectFirst {
case womType if womType.coerceRawValue(value).isSuccess => womType.coerceRawValue(value).get
} getOrElse {
BadDefaultAttribute(WomString(value.toString))

Check warning on line 136 in backend/src/main/scala/cromwell/backend/validation/TwoKeyRuntimeAttributesValidation.scala

View check run for this annotation

Codecov / codecov/patch

backend/src/main/scala/cromwell/backend/validation/TwoKeyRuntimeAttributesValidation.scala#L136

Added line #L136 was not covered by tests
}
}

/**
* Returns this as an instance of a runtime attribute definition.
*/
final lazy val altKeyRuntimeAttributeDefinition =
RuntimeAttributeDefinition(altKey, staticDefaultOption, usedInCallCaching)
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import wom.expression.WomExpression
import wom.types.WomType
import wom.values.WomValue

import scala.collection.mutable.ListBuffer
import scala.util.control.NoStackTrace

final case class ValidatedRuntimeAttributes(attributes: Map[String, Any])
Expand All @@ -33,7 +34,15 @@ trait ValidatedRuntimeAttributesBuilder {
/**
* Returns a mapping of the validations: RuntimeAttributesValidation each converted to a RuntimeAttributeDefinition.
*/
final lazy val definitions: Seq[RuntimeAttributeDefinition] = validations.map(_.runtimeAttributeDefinition)
final lazy val definitions: Seq[RuntimeAttributeDefinition] =
validations.map(_.runtimeAttributeDefinition) ++ validations
.map {
case value: TwoKeyRuntimeAttributesValidation[_, _] =>
value.altKeyRuntimeAttributeDefinition
case _ =>
null
rsaperst marked this conversation as resolved.
Show resolved Hide resolved
}
.filterNot(x => x == null)

/**
* Returns validators suitable for BackendWorkflowInitializationActor.runtimeAttributeValidators.
Expand All @@ -49,7 +58,14 @@ trait ValidatedRuntimeAttributesBuilder {

def unsupportedKeys(keys: Seq[String]): Seq[String] = keys.diff(validationKeys)

private lazy val validationKeys = validations.map(_.key)
private lazy val validationKeys = validations.map(_.key) ++ validations
.map {
case value: TwoKeyRuntimeAttributesValidation[_, _] =>
value.altKey
case _ =>
null
}
.filterNot(x => x == null)

def build(attrs: Map[String, WomValue], logger: Logger): ValidatedRuntimeAttributes = {
RuntimeAttributesValidation.warnUnrecognized(attrs.keySet, validationKeys.toSet, logger)
Expand All @@ -67,11 +83,23 @@ trait ValidatedRuntimeAttributesBuilder {
}

private def validate(values: Map[String, WomValue]): ErrorOr[ValidatedRuntimeAttributes] = {
val listOfKeysToErrorOrAnys: List[(String, ErrorOr[Any])] =
validations.map(validation => validation.key -> validation.validate(values)).toList
val listOfKeysToErrorOrAnys: ListBuffer[(String, ErrorOr[Any])] = ListBuffer()

validations.foreach { validation =>
listOfKeysToErrorOrAnys += validation.key -> validation.validate(values)

validation match {
case twoKeyValidation: TwoKeyRuntimeAttributesValidation[_, _] =>
if (values.contains(twoKeyValidation.altKey)) {
listOfKeysToErrorOrAnys += twoKeyValidation.altKey -> twoKeyValidation.validateAltKey(values)
}
case _ =>
}
}

val listOfErrorOrKeysToAnys: List[ErrorOr[(String, Any)]] = listOfKeysToErrorOrAnys map { case (key, errorOrAny) =>
errorOrAny map { any => (key, any) }
val listOfErrorOrKeysToAnys: List[ErrorOr[(String, Any)]] = listOfKeysToErrorOrAnys.toList map {
case (key, errorOrAny) =>
errorOrAny map { any => (key, any) }
}

import cats.syntax.traverse._
Expand Down
Loading
Loading