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.

Sign up for GitHub

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

AN-349 AN-358 Fix cost status #7676

Merged
merged 8 commits into from
Jan 15, 2025
Merged

AN-349 AN-358 Fix cost status #7676

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b6d7108
Use original status when building cost response
jgainerdewar Jan 13, 2025
150435e
Update test to cover bug
jgainerdewar Jan 13, 2025
f4e6143
Set up sbt in GHA
jgainerdewar Jan 13, 2025
4fadf86
Set up sbt in more places
jgainerdewar Jan 13, 2025
c79b0e8
Attempt pip fix
jgainerdewar Jan 14, 2025
e1abf4d
Comments
jgainerdewar Jan 14, 2025
169261b
Pip comment
jgainerdewar Jan 14, 2025
13588b2
Disable slurm test temporarily
jgainerdewar Jan 15, 2025
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
1 change: 1 addition & 0 deletions .github/workflows/cromwell_unit_tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ jobs:

steps:
- uses: actions/checkout@v3 # checkout the cromwell repo
- uses: sbt/setup-sbt@v1
- uses: ./.github/set_up_cromwell_action #Exectute this reusable github action. It will set up java/sbt/git-secrets/cromwell.
with:
cromwell_repo_token: ${{ secrets.BROADBOT_GITHUB_TOKEN }}
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/docker_build_test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ jobs:
repository: broadinstitute/cromwell
token: ${{ secrets.BROADBOT_GITHUB_TOKEN }}
path: cromwell
- uses: sbt/setup-sbt@v1
- uses: actions/setup-java@v4
with:
distribution: 'temurin'
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/integration_tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ jobs:
runs-on: ubuntu-latest
timeout-minutes: 120
steps:
- uses: sbt/setup-sbt@v1
- uses: actions/checkout@v3 # checkout the cromwell repo
with:
ref: ${{ inputs.target-branch }}
Expand Down
21 changes: 14 additions & 7 deletions engine/src/test/scala/cromwell/webservice/MetadataBuilderActorSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,8 @@ class MetadataBuilderActorSpec
val subWorkflow1aId = WorkflowId(UUID.fromString("1a1a1a1a-f76d-4af3-b371-5ba580916729"))
val subWorkflow1bId = WorkflowId(UUID.fromString("1b1b1b1b-f76d-4af3-b371-5ba580916729"))

val workflowState = WorkflowSucceeded
val workflowSucceededState = WorkflowSucceeded
val workflowRunningState = WorkflowRunning
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed that this test fails on develop


val mainEvents = List(
MetadataEvent(MetadataKey(mainWorkflowId, Option(MetadataJobKey("wfMain", None, 1)), "subWorkflowId"),
Expand Down Expand Up @@ -391,19 +392,25 @@ class MetadataBuilderActorSpec
class TestReadDatabaseMetadataWorkerActorForCost extends ReadDatabaseMetadataWorkerActor(defaultTimeout, 1000000) {
override def receive: Receive = {
case GetCost(wfId) if wfId == mainWorkflowId =>
sender() ! CostResponse(mainWorkflowId, workflowState, MetadataLookupResponse(mainQuery, mainEvents))
sender() ! CostResponse(mainWorkflowId, workflowRunningState, MetadataLookupResponse(mainQuery, mainEvents))
()
case GetCost(wfId) if wfId == subWorkflow1Id =>
sender() ! CostResponse(subWorkflow1Id, workflowState, MetadataLookupResponse(sub1Query, sub1Events))
sender() ! CostResponse(subWorkflow1Id, workflowSucceededState, MetadataLookupResponse(sub1Query, sub1Events))
()
case GetCost(wfId) if wfId == subWorkflow2Id =>
sender() ! CostResponse(subWorkflow2Id, workflowState, MetadataLookupResponse(sub2Query, sub2Events))
sender() ! CostResponse(subWorkflow2Id, workflowSucceededState, MetadataLookupResponse(sub2Query, sub2Events))
()
case GetCost(wfId) if wfId == subWorkflow1aId =>
sender() ! CostResponse(subWorkflow1aId, workflowState, MetadataLookupResponse(sub1aQuery, sub1aEvents))
sender() ! CostResponse(subWorkflow1aId,
Copy link

Choose a reason for hiding this comment

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

Nothing to do really and naming things are hard anyways, but I had to reread this 3 times before I noticed the 1, vs 1a, vs 1b, vs 2 😓

workflowSucceededState,
MetadataLookupResponse(sub1aQuery, sub1aEvents)
)
()
case GetCost(wfId) if wfId == subWorkflow1bId =>
sender() ! CostResponse(subWorkflow1bId, workflowState, MetadataLookupResponse(sub1bQuery, sub1bEvents))
sender() ! CostResponse(subWorkflow1bId,
workflowSucceededState,
MetadataLookupResponse(sub1bQuery, sub1bEvents)
)
()
case _ => ()
}
Expand All @@ -414,7 +421,7 @@ class MetadataBuilderActorSpec
|"cost": 7,
|"currency": "USD",
|"id": "${mainWorkflowId}",
|"status": "${workflowState.toString}",
|"status": "${workflowRunningState.toString}",
|"errors": ["Couldn't find valid vmCostPerHour for call1aA.-1.1"]
|}""".stripMargin

Expand Down
50 changes: 36 additions & 14 deletions services/src/main/scala/cromwell/services/metadata/impl/builder/MetadataBuilderActor.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,17 +35,38 @@ object MetadataBuilderActor {
case object IdleData extends MetadataBuilderActorData
final case class HasWorkData(target: ActorRef, originalRequest: BuildMetadataJsonAction)
extends MetadataBuilderActorData

sealed trait EventsCollectorData extends MetadataBuilderActorData {
val target: ActorRef
val originalRequest: BuildMetadataJsonAction
val originalQuery: MetadataQuery
val originalEvents: Seq[MetadataEvent]
val subWorkflowsMetadata: Map[String, JsValue]
val waitFor: Int

def isComplete = subWorkflowsMetadata.size == waitFor
}
final case class HasReceivedEventsData(target: ActorRef,
originalRequest: BuildMetadataJsonAction,
originalQuery: MetadataQuery,
originalEvents: Seq[MetadataEvent],
subWorkflowsMetadata: Map[String, JsValue],
waitFor: Int
) extends MetadataBuilderActorData {
) extends EventsCollectorData {
def withSubWorkflow(id: String, metadata: JsValue) =
this.copy(subWorkflowsMetadata = subWorkflowsMetadata + ((id, metadata)))
}

def isComplete = subWorkflowsMetadata.size == waitFor
final case class HasReceivedCostEventsData(target: ActorRef,
Copy link
Collaborator Author

@jgainerdewar jgainerdewar Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Previously we used HasReceivedEventsData as the data object when collecting per-subworkflow results for either plain metadata or cost. I'm splitting it into two classes here for the two different use cases, so we can include workflow status in the cost one.

Opinions welcome on the naming - I find HasReceivedEventsData to be an unclear name for that object, but didn't want to introduce churn by changing it, so I tried to find a sensible-seeming name for a new parent trait.

Copy link

Choose a reason for hiding this comment

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

Honestly, i find that leaving an inline comment before the parent trait and then before the class to be more useful than trying to name things perfectly

originalRequest: BuildMetadataJsonAction,
originalQuery: MetadataQuery,
originalEvents: Seq[MetadataEvent],
originalStatus: WorkflowState,
subWorkflowsMetadata: Map[String, JsValue],
waitFor: Int
) extends EventsCollectorData {
def withSubWorkflow(id: String, metadata: JsValue) =
this.copy(subWorkflowsMetadata = subWorkflowsMetadata + ((id, metadata)))
}

def props(readMetadataWorkerMaker: () => Props,
Expand Down Expand Up @@ -432,7 +453,7 @@ class MetadataBuilderActor(readMetadataWorkerMaker: () => Props,
}

when(WaitingForSubWorkflowCost) {
case Event(mbr: MetadataJsonResponse, data: HasReceivedEventsData) =>
case Event(mbr: MetadataJsonResponse, data: HasReceivedCostEventsData) =>
processSubWorkflowCost(mbr, data)
case Event(failure: MetadataServiceFailure, data: HasReceivedEventsData) =>
data.target ! FailedMetadataJsonResponse(data.originalRequest, failure.reason)
Expand Down Expand Up @@ -482,7 +503,7 @@ class MetadataBuilderActor(readMetadataWorkerMaker: () => Props,
failAndDie(new Exception(message), data.target, data.originalRequest)
}

def processSubWorkflowCost(metadataResponse: MetadataJsonResponse, data: HasReceivedEventsData) =
def processSubWorkflowCost(metadataResponse: MetadataJsonResponse, data: HasReceivedCostEventsData) =
metadataResponse match {
case SuccessfulMetadataJsonResponse(GetCost(workflowId), js) =>
val subId: WorkflowId = workflowId
Expand All @@ -491,7 +512,7 @@ class MetadataBuilderActor(readMetadataWorkerMaker: () => Props,
if (newData.isComplete) {
buildCostAndStop(
data.originalQuery.workflowId,
extractFromJsAs[JsString](js, "status").map(_.value).getOrElse(""), // should never be empty
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Very happy to delete this terrible line of code that past me wrote.

data.originalStatus,
data.originalEvents,
newData.subWorkflowsMetadata,
data.target,
Expand Down Expand Up @@ -579,7 +600,7 @@ class MetadataBuilderActor(readMetadataWorkerMaker: () => Props,

if (subWorkflowIds.isEmpty)
// If no subworkflows found, just build cost data
buildCostAndStop(id, status.toString, metadataResponse.eventList, Map.empty, target, originalRequest)
buildCostAndStop(id, status, metadataResponse.eventList, Map.empty, target, originalRequest)
else {
// Otherwise spin up a metadata builder actor for each sub workflow
subWorkflowIds foreach { subId =>
Expand All @@ -591,18 +612,19 @@ class MetadataBuilderActor(readMetadataWorkerMaker: () => Props,
)
subMetadataBuilder ! GetCost(WorkflowId.fromString(subId))
}
goto(WaitingForSubWorkflowCost) using HasReceivedEventsData(target,
originalRequest,
metadataResponse.query,
metadataResponse.eventList,
Map.empty,
subWorkflowIds.size
goto(WaitingForSubWorkflowCost) using HasReceivedCostEventsData(target,
originalRequest,
metadataResponse.query,
metadataResponse.eventList,
status,
Map.empty,
subWorkflowIds.size
)
}
}

def buildCostAndStop(id: WorkflowId,
status: String,
status: WorkflowState,
eventsList: Seq[MetadataEvent],
expandedValues: Map[String, JsValue],
target: ActorRef,
Expand Down Expand Up @@ -644,7 +666,7 @@ class MetadataBuilderActor(readMetadataWorkerMaker: () => Props,
val resp = JsObject(
Map(
WorkflowMetadataKeys.Id -> JsString(id.toString),
WorkflowMetadataKeys.Status -> JsString(status),
WorkflowMetadataKeys.Status -> JsString(status.toString),
"currency" -> JsString(DefaultCurrency.getCurrencyCode),
"cost" -> JsNumber(callCost + subworkflowCost),
"errors" -> JsArray(costErrors ++ subworkflowErrors)
Expand Down
Loading