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

Credit system for jobs #8352

Draft
wants to merge 60 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
308528d
WIP: implement credit system
Jan 21, 2025
be4a9e3
Merge branch 'master' of github.com:scalableminds/webknossos into cre…
Jan 22, 2025
e81961d
finish public writes method for creditTransaction
Jan 22, 2025
6c7a09c
First version of backend for credit transaction system
Jan 22, 2025
1606c54
IP: add frontend part
Jan 22, 2025
20c51b7
WIP: implement frontend part
Jan 23, 2025
9c9d868
first frontend version & add proper backend credit ordering route
Jan 24, 2025
2fc2ef2
fix import sorting frontend
Jan 24, 2025
4bc0f6b
format backend & fix auto refunding of failed jobs
Jan 24, 2025
c763782
Merge branch 'master' of github.com:scalableminds/webknossos into cre…
Jan 24, 2025
e8a9ca4
refresh snapshots & fix schema
Jan 24, 2025
0399afb
skip CI
Jan 24, 2025
8eb443f
fix schema
Jan 24, 2025
98164a9
skip reversion check
Jan 24, 2025
a6b990c
remove unused implicit dbcontexts
Jan 24, 2025
e65c7c9
Merge branch 'master' of github.com:scalableminds/webknossos into cre…
Jan 27, 2025
c284fac
Wrap refunding a transaction in a db transaction to avoid race condit…
Jan 27, 2025
d8f5eb0
remove unused imports
Jan 27, 2025
2de17f7
make job costs bigdecimal
Jan 27, 2025
6982c7f
extract job payment functionallity and ensure if starting a job faile…
Jan 27, 2025
5e18319
Ensure ordering credits is strongly positive
Jan 27, 2025
9a1d70a
re enable ci
Jan 27, 2025
8f08491
warp revoking stored procedure in transaction
Jan 27, 2025
cef7886
add missing transaction state
Jan 27, 2025
48b3e58
add db reversion
Jan 28, 2025
68b7707
resync evolution file with schema.sql
Jan 28, 2025
722d14d
add migrations to migrations doc unreleased
Jan 28, 2025
56e09aa
format backend
Jan 28, 2025
d9639a5
fix frontend typing
Jan 28, 2025
a5d3069
fix jobcontroller & refunding credits & clear some mini TODOs
Jan 28, 2025
c695d51
Merge branch 'master' of github.com:scalableminds/webknossos into cre…
Jan 29, 2025
a11076d
do not consider updated row twice when ensuring non negative balance
Jan 29, 2025
125726f
remove logging
Jan 29, 2025
2fa527f
schedule revoking credits from within webknossos backend
Jan 29, 2025
4819f69
auto refetch organization upon starting a paid job to get up to date …
Jan 29, 2025
48b84cb
auto refetch organization upon starting a paid job to get up to date …
Jan 29, 2025
57369ff
fix frontend import sorting
Jan 30, 2025
63b21cd
round credit balance shown to user to max 2 digits after comma
Jan 30, 2025
eb573d0
fix grammar in order wk credits request email
Jan 30, 2025
e05c6db
WIP: implement credit revoking in scala
Jan 30, 2025
d35fcb8
Further work on credit revoking
Jan 30, 2025
a2e0f01
- refactor credit revoking
Feb 3, 2025
b0dd64e
Make credits revoking work with transactionally
Feb 3, 2025
86bf76b
remove unused imports
Feb 3, 2025
7b4faa7
try fixing schema
Feb 3, 2025
c42a53d
fix schema
Feb 3, 2025
517b643
fix schema
Feb 3, 2025
6fd95c0
fetch job costs from backend for improved precision & ensuring same c…
Feb 3, 2025
4842102
remove unused val
Feb 3, 2025
0cf02d0
fix bug in changed datasets detection in frontend
Feb 3, 2025
7647ba1
fix trigger ensuring non negative balance
Feb 3, 2025
9c6acb9
fix insert transaction route
Feb 3, 2025
a2abad2
Merge branch 'master' of github.com:scalableminds/webknossos into cre…
Feb 3, 2025
c2fae41
make job naming in frontend more consistent with backend
Feb 3, 2025
6acaaa0
format frontend
Feb 3, 2025
fd2a1e2
fix neuron inferral preview image
Feb 3, 2025
7fcd4ee
add route to trigger free credits revoking mechanism
Feb 3, 2025
1ab1a4b
fix resvering credits for a job
Feb 3, 2025
f854c74
fix revoking free credits
Feb 4, 2025
b2aa2f8
fix revoking free credits
Feb 4, 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
2 changes: 1 addition & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ jobs:
docker-compose run compile tools/postgres/dbtool.js check-evolutions-schema
- run:
name: Assert that all migrations are mentioned in one migration guide and that they have a reversion counterpart.
command: tools/assert-complete-migrations.sh
command: .circleci/not-on-master.sh tools/assert-complete-migrations.sh
MichaelBuessemeyer marked this conversation as resolved.
Show resolved Hide resolved
- run:
name: Build frontend documentation
command: |
Expand Down
3 changes: 2 additions & 1 deletion .circleci/not-on-master.sh
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@ set -Eeuo pipefail
if [ "${CIRCLE_BRANCH}" == "master" ]; then
echo "Skipping this step on master..."
else
exec "$@"
echo "Skipping everywhere..."
# exec "$@"
MichaelBuessemeyer marked this conversation as resolved.
Show resolved Hide resolved
fi
68 changes: 68 additions & 0 deletions app/controllers/CreditTransactionController.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package controllers

import com.scalableminds.util.objectid.ObjectId
import com.scalableminds.util.time.Instant
import com.scalableminds.util.tools.{Fox, FoxImplicits}
import models.organization.{
CreditTransaction,
CreditTransactionDAO,
CreditTransactionService,
CreditTransactionState,
OrganizationService
}
import models.user.UserService
import net.liftweb.common.Box.tryo
import play.api.mvc.{Action, AnyContent}
import play.silhouette.api.Silhouette
import security.WkEnv

import javax.inject.Inject
import scala.concurrent.ExecutionContext

class CreditTransactionController @Inject()(organizationService: OrganizationService,
creditTransactionService: CreditTransactionService,
creditTransactionDAO: CreditTransactionDAO,
userService: UserService,
sil: Silhouette[WkEnv])(implicit ec: ExecutionContext)
extends Controller
with FoxImplicits {

def chargeUpCredits(organizationId: String,
creditAmount: Int,
moneySpent: String,
comment: Option[String],
expiresAt: Option[String]): Action[AnyContent] = sil.SecuredAction.async { implicit request =>
for {
_ <- userService.assertIsSuperUser(request.identity) ?~> "Only super users can charge up credits"
moneySpentInDecimal <- tryo(BigDecimal(moneySpent)) ?~> s"moneySpent $moneySpent is not a valid decimal"
_ <- bool2Fox(moneySpentInDecimal > 0) ?~> "moneySpent must be a positive number"
_ <- bool2Fox(creditAmount > 0) ?~> "creditAmount must be a positive number"
commentNoOptional = comment.getOrElse(s"Charge up for $creditAmount credits for $moneySpent Euro.")
_ <- organizationService.ensureOrganizationHasPaidPlan(organizationId)
expirationDateOpt <- Fox.runOptional(expiresAt)(Instant.fromString)
chargeUpTransaction = CreditTransaction(
ObjectId.generate,
organizationId,
BigDecimal(creditAmount),
Some(moneySpentInDecimal),
commentNoOptional,
None,
CreditTransactionState.Completed,
expirationDateOpt
)
_ <- creditTransactionService.doCreditTransaction(chargeUpTransaction)
} yield Ok
}
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 24, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Handle potential concurrency issues and partial rollback.
Though the code validates user permissions and input boundaries (e.g., creditAmount > 0), concurrent invocations of chargeUpCredits could lead to race conditions if the design expects real-time balance tracking. Consider using database transactions or locks for robust concurrency handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not currently relevant to our domain. Charge-ups happen rarely and in case a user reads outdated values, this isn't a problem to use as long as the data in the database is consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

@MichaelBuessemeyer Understood, thank you for the clarification. I'll note this for future reviews.


✏️ Learnings added
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/controllers/CreditTransactionController.scala:30-55
Timestamp: 2025-01-27T12:51:23.807Z
Learning: In the WEBKNOSSOS credit system, charge-up operations are infrequent and temporary outdated credit balance reads are acceptable. Database-level consistency is sufficient, and additional concurrency control mechanisms are not required.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!


def refundCreditTransaction(organizationId: String, transactionId: String): Action[AnyContent] =
sil.SecuredAction.async { implicit request =>
for {
_ <- userService.assertIsSuperUser(request.identity) ?~> "Only super users can manually refund credits"
transaction <- creditTransactionDAO.findOne(transactionId)
_ <- bool2Fox(transaction._organization == organizationId) ?~> "Transaction is not for this organization"
_ <- organizationService.ensureOrganizationHasPaidPlan(organizationId)
_ <- creditTransactionDAO.refundTransaction(transactionId)
} yield Ok
}

}
46 changes: 41 additions & 5 deletions app/controllers/JobController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import com.scalableminds.util.accesscontext.GlobalAccessContext
import com.scalableminds.util.tools.Fox
import models.dataset.{DataStoreDAO, DatasetDAO, DatasetLayerAdditionalAxesDAO, DatasetService}
import models.job._
import models.organization.OrganizationDAO
import models.organization.{CreditTransactionService, OrganizationDAO, OrganizationService}
import models.user.MultiUserDAO
import play.api.i18n.Messages
import play.api.libs.json._
Expand Down Expand Up @@ -64,6 +64,8 @@ class JobController @Inject()(
wkSilhouetteEnvironment: WkSilhouetteEnvironment,
slackNotificationService: SlackNotificationService,
organizationDAO: OrganizationDAO,
organizationService: OrganizationService,
creditTransactionService: CreditTransactionService,
dataStoreDAO: DataStoreDAO)(implicit ec: ExecutionContext, playBodyParsers: PlayBodyParsers)
extends Controller
with Zarr3OutputHelper {
Expand All @@ -85,6 +87,7 @@ class JobController @Inject()(
for {
_ <- bool2Fox(wkconf.Features.jobsEnabled) ?~> "job.disabled"
jobs <- jobDAO.findAll
// TODO: Consider adding paid credits to job public writes.
jobsJsonList <- Fox.serialCombined(jobs.sortBy(_.created).reverse)(jobService.publicWrites)
} yield Ok(Json.toJson(jobsJsonList))
}
Expand Down Expand Up @@ -233,11 +236,16 @@ class JobController @Inject()(
"organization.notFound",
dataset._organization)
_ <- bool2Fox(request.identity._organization == organization._id) ?~> "job.inferNeurons.notAllowed.organization" ~> FORBIDDEN
_ <- organizationService.ensureOrganizationHasPaidPlan(organization._id)
_ <- datasetService.assertValidDatasetName(newDatasetName)
_ <- datasetService.assertValidLayerNameLax(layerName)
multiUser <- multiUserDAO.findOne(request.identity._multiUser)
_ <- Fox.runIf(!multiUser.isSuperUser)(jobService.assertBoundingBoxLimits(bbox, None))
command = JobCommand.infer_neurons
parsedBoundingBox <- jobService.parseBoundingBoxWithMagOpt(bbox, None)
costsInCredits = jobService.calculateJobCosts(parsedBoundingBox, command)
_ <- Fox.assertTrue(creditTransactionService.hasEnoughCredits(organization._id, costsInCredits)) ?~> "job.notEnoughCredits" // TODO: add error message to messages.conf
// TODO: Disable this check. Credits should be enough to guard this.
_ <- Fox.runIf(!multiUser.isSuperUser)(jobService.assertBoundingBoxLimits(bbox, None))
commandArgs = Json.obj(
"organization_id" -> organization._id,
"dataset_name" -> dataset.name,
Expand All @@ -246,7 +254,13 @@ class JobController @Inject()(
"layer_name" -> layerName,
"bbox" -> bbox,
)
creditTransactionComment = s"Spent $costsInCredits for AI neuron segmentation for dataset ${dataset.name}"
creditTransaction <- creditTransactionService.reserveCredits(organization._id,
costsInCredits,
creditTransactionComment,
None)
job <- jobService.submitJob(command, commandArgs, request.identity, dataset._dataStore) ?~> "job.couldNotRunNeuronInferral"
_ <- creditTransactionService.addJobIdToTransaction(creditTransaction, job._id)
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Handle potential partial failures after reserving credits
Currently, if reserveCredits succeeds but the job submission fails, credits remain reserved. Consider implementing a rollback or a two-phase commit mechanism to handle partial failures. Storing more structured metadata in the transaction might also help with future auditing or debugging.

js <- jobService.publicWrites(job)
} yield Ok(js)
}
Expand All @@ -268,9 +282,13 @@ class JobController @Inject()(
_ <- datasetService.assertValidDatasetName(newDatasetName)
_ <- datasetService.assertValidLayerNameLax(layerName)
multiUser <- multiUserDAO.findOne(request.identity._multiUser)
command = JobCommand.infer_mitochondria
parsedBoundingBox <- jobService.parseBoundingBoxWithMagOpt(bbox, None)
costsInCredits = jobService.calculateJobCosts(parsedBoundingBox, command)
_ <- Fox.assertTrue(creditTransactionService.hasEnoughCredits(organization._id, costsInCredits)) ?~> "job.notEnoughCredits" // TODO: add error message to messages.conf
// TODO: Disable this check. Credits should be enough to guard this.
_ <- bool2Fox(multiUser.isSuperUser) ?~> "job.inferMitochondria.notAllowed.onlySuperUsers"
_ <- Fox.runIf(!multiUser.isSuperUser)(jobService.assertBoundingBoxLimits(bbox, None))
command = JobCommand.infer_mitochondria
commandArgs = Json.obj(
"organization_id" -> dataset._organization,
"dataset_name" -> dataset.name,
Expand All @@ -279,7 +297,13 @@ class JobController @Inject()(
"layer_name" -> layerName,
"bbox" -> bbox,
)
creditTransactionComment = s"Spent $costsInCredits for AI mitochondria segmentation for dataset ${dataset.name}"
creditTransaction <- creditTransactionService.reserveCredits(organization._id,
costsInCredits,
creditTransactionComment,
None)
job <- jobService.submitJob(command, commandArgs, request.identity, dataset._dataStore) ?~> "job.couldNotRunInferMitochondria"
_ <- creditTransactionService.addJobIdToTransaction(creditTransaction, job._id)
js <- jobService.publicWrites(job)
} yield Ok(js)
}
Expand All @@ -300,8 +324,14 @@ class JobController @Inject()(
_ <- bool2Fox(request.identity._organization == organization._id) ?~> "job.alignSections.notAllowed.organization" ~> FORBIDDEN
_ <- datasetService.assertValidDatasetName(newDatasetName)
_ <- datasetService.assertValidLayerNameLax(layerName)
datasetBoundingBox <- datasetService
.dataSourceFor(dataset)
.flatMap(_.toUsable)
.map(_.boundingBox) ?~> "dataset.boundingBox.unset" // TODO: add error message to messages.conf
_ <- Fox.runOptional(annotationId)(ObjectId.fromString)
command = JobCommand.align_sections
costsInCredits = jobService.calculateJobCosts(datasetBoundingBox, command)
_ <- Fox.assertTrue(creditTransactionService.hasEnoughCredits(organization._id, costsInCredits)) ?~> "job.notEnoughCredits" // TODO: add error message to messages.conf
commandArgs = Json.obj(
"organization_id" -> organization._id,
"dataset_name" -> dataset.name,
Expand All @@ -310,7 +340,13 @@ class JobController @Inject()(
"layer_name" -> layerName,
"annotation_id" -> annotationId
)
creditTransactionComment = s"Spent $costsInCredits for AI neuron segmentation for dataset ${dataset.name}"
creditTransaction <- creditTransactionService.reserveCredits(organization._id,
costsInCredits,
creditTransactionComment,
None)
job <- jobService.submitJob(command, commandArgs, request.identity, dataset._dataStore) ?~> "job.couldNotRunAlignSections"
_ <- creditTransactionService.addJobIdToTransaction(creditTransaction, job._id)
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Reduce duplication in credit reservation
Like other job setup methods, the transaction handling here repeats the same pattern. Factor out the code for calculating costs, reserving credits, and linking the transaction to a job to avoid errors and ease maintenance.

js <- jobService.publicWrites(job)
} yield Ok(js)
}
Expand Down Expand Up @@ -454,10 +490,10 @@ class JobController @Inject()(
_ <- bool2Fox(request.identity._organization == organization._id) ?~> "job.renderAnimation.notAllowed.organization" ~> FORBIDDEN
userOrganization <- organizationDAO.findOne(request.identity._organization)
animationJobOptions = request.body
_ <- Fox.runIf(userOrganization.pricingPlan == PricingPlan.Basic) {
_ <- Fox.runIf(PricingPlan.isPaidPlan(userOrganization.pricingPlan)) {
bool2Fox(animationJobOptions.includeWatermark) ?~> "job.renderAnimation.mustIncludeWatermark"
}
_ <- Fox.runIf(userOrganization.pricingPlan == PricingPlan.Basic) {
_ <- Fox.runIf(PricingPlan.isPaidPlan(userOrganization.pricingPlan)) {
bool2Fox(animationJobOptions.movieResolution == MovieResolutionSetting.SD) ?~> "job.renderAnimation.resolutionMustBeSD"
}
layerName = animationJobOptions.layerName
Expand Down
15 changes: 15 additions & 0 deletions app/controllers/OrganizationController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,21 @@ class OrganizationController @Inject()(
} yield Ok
}

def sendOrderCreditsEmail(requestedCredits: Int): Action[AnyContent] =
sil.SecuredAction.async { implicit request =>
for {
_ <- bool2Fox(request.identity.isOrganizationOwner) ?~> Messages("organization.creditOrder.notAuthorized")
organization <- organizationDAO.findOne(request.identity._organization) ?~> Messages("organization.notFound") ~> NOT_FOUND
userEmail <- userService.emailFor(request.identity)
_ = Mailer ! Send(defaultMails.orderCreditsMail(request.identity, userEmail, requestedCredits))
_ = Mailer ! Send(
defaultMails.orderCreditsRequestMail(request.identity,
userEmail,
organization.name,
s"Purchase $requestedCredits WEBKNOSSOS credits."))
} yield Ok
}

Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Validate request parameters and add logging

The new sendOrderCreditsEmail method handles ordering credits by sending two email messages. To improve robustness, consider validating that requestedCredits is a positive integer to prevent invalid requests, and add some logging to confirm that the request to order credits was successfully initiated or if any exception occurred.

def pricingStatus: Action[AnyContent] =
sil.SecuredAction.async { implicit request =>
for {
Expand Down
8 changes: 8 additions & 0 deletions app/controllers/WKRemoteWorkerController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import models.job.JobCommand.JobCommand

import javax.inject.Inject
import models.job._
import models.organization.CreditTransactionService
import models.voxelytics.VoxelyticsDAO
import net.liftweb.common.{Empty, Failure, Full}
import play.api.libs.json.Json
Expand All @@ -20,6 +21,7 @@ import scala.concurrent.ExecutionContext
class WKRemoteWorkerController @Inject()(jobDAO: JobDAO,
jobService: JobService,
workerDAO: WorkerDAO,
creditTransactionService: CreditTransactionService,
voxelyticsDAO: VoxelyticsDAO,
aiInferenceDAO: AiInferenceDAO,
datasetDAO: DatasetDAO,
Expand Down Expand Up @@ -82,6 +84,12 @@ class WKRemoteWorkerController @Inject()(jobDAO: JobDAO,
jobAfterChange <- jobDAO.findOne(jobIdParsed)(GlobalAccessContext)
_ = jobService.trackStatusChange(jobBeforeChange, jobAfterChange)
_ <- jobService.cleanUpIfFailed(jobAfterChange)
_ <- Fox.runIf(request.body.state == JobState.SUCCESS) {
creditTransactionService.completeTransactionOfJob(jobAfterChange._id)(GlobalAccessContext)
}
_ <- Fox.runIf(request.body.state == JobState.FAILURE) {
creditTransactionService.refundTransactionForJob(jobAfterChange._id)(GlobalAccessContext)
}
} yield Ok
}

Expand Down
17 changes: 17 additions & 0 deletions app/mail/DefaultMails.scala
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,23 @@ class DefaultMails @Inject()(conf: WkConf) {
recipients = List("[email protected]")
)

def orderCreditsMail(user: User, userEmail: String, requestedCredits: Int): Mail =
Mail(
from = defaultSender,
subject = "Request to order WEBKNOSSOS credits",
bodyHtml = html.mail.orderCredits(user.name, requestedCredits, additionalFooter).body,
recipients = List(userEmail)
)

def orderCreditsRequestMail(user: User, userEmail: String, organizationName: String, messageBody: String): Mail =
Mail(
from = defaultSender,
subject = "Request to buy WEBKNOSSOS credits",
bodyHtml =
html.mail.orderCreditsRequest(user.name, userEmail, organizationName, messageBody, additionalFooter).body,
recipients = List("[email protected]")
)

def jobSuccessfulGenericMail(user: User,
userEmail: String,
datasetName: String,
Expand Down
25 changes: 22 additions & 3 deletions app/models/job/JobService.scala
Original file line number Diff line number Diff line change
Expand Up @@ -219,11 +219,30 @@ class JobService @Inject()(wkConf: WkConf,

def assertBoundingBoxLimits(boundingBox: String, mag: Option[String]): Fox[Unit] =
for {
parsedBoundingBox <- BoundingBox.fromLiteral(boundingBox).toFox ?~> "job.invalidBoundingBox"
parsedMag <- Vec3Int.fromMagLiteral(mag.getOrElse("1-1-1"), allowScalar = true) ?~> "job.invalidMag"
boundingBoxInMag = parsedBoundingBox / parsedMag
boundingBoxInMag <- parseBoundingBoxWithMagOpt(boundingBox, mag)
_ <- bool2Fox(boundingBoxInMag.volume <= wkConf.Features.exportTiffMaxVolumeMVx * 1024 * 1024) ?~> "job.volumeExceeded"
_ <- bool2Fox(boundingBoxInMag.size.maxDim <= wkConf.Features.exportTiffMaxEdgeLengthVx) ?~> "job.edgeLengthExceeded"
} yield ()

def getJobCostsPerGVx(jobCommand: JobCommand): Double =
jobCommand match {
case JobCommand.infer_neurons => wkConf.Features.neuronInferralCostsPerGVx
case JobCommand.infer_mitochondria => wkConf.Features.mitochondriaInferralCostsPerGVx
case JobCommand.align_sections => wkConf.Features.alignmentCostsPerGVx
case _ => throw new IllegalArgumentException(s"Unsupported job command $jobCommand")
}

def calculateJobCosts(boundingBoxInTargetMag: BoundingBox, jobCommand: JobCommand): BigDecimal = {
val costsPerGVx = getJobCostsPerGVx(jobCommand)
val volumeInGVx = boundingBoxInTargetMag.volume / math.pow(10, 9)
val costs = BigDecimal(volumeInGVx) * costsPerGVx
costs
}

Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use consistent numeric types for cost calculations.

Currently, getJobCostsPerGVx returns a Double, which is then converted to BigDecimal. This can cause precision inconsistencies. Prefer storing costs in BigDecimal from the start to maintain numeric consistency.

def parseBoundingBoxWithMagOpt(boundingBox: String, mag: Option[String]): Fox[BoundingBox] =
for {
parsedBoundingBox <- BoundingBox.fromLiteral(boundingBox).toFox ?~> "job.invalidBoundingBox"
parsedMag <- Vec3Int.fromMagLiteral(mag.getOrElse("1-1-1"), allowScalar = true) ?~> "job.invalidMag"
} yield parsedBoundingBox / parsedMag

}
Loading