-
Notifications
You must be signed in to change notification settings - Fork 10
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
[risk=low][no ticket] Logic changes for runtime cost display #8980
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,20 @@ | ||
import { CSSProperties } from 'react'; | ||
|
||
import { cond } from '@terra-ui-packages/core-utils'; | ||
import { PersistentDiskRequest } from 'generated/fetch'; | ||
|
||
import { FlexColumn, FlexRow } from 'app/components/flex'; | ||
import { TooltipTrigger } from 'app/components/popups'; | ||
import colors from 'app/styles/colors'; | ||
import { reactStyles } from 'app/utils'; | ||
import { AnalysisConfig } from 'app/utils/analysis-config'; | ||
import { | ||
detachableDiskPricePerMonth, | ||
diskConfigPricePerMonth, | ||
machineRunningCost, | ||
derivePdFromAnalysisConfig, | ||
machineRunningCostBreakdown, | ||
machineStorageCost, | ||
machineRunningCostPerHour, | ||
machineStorageCostBreakdown, | ||
machineStorageCostPerHour, | ||
persistentDiskPricePerMonth, | ||
RunningCost, | ||
} from 'app/utils/machines'; | ||
import { formatUsd } from 'app/utils/numbers'; | ||
|
||
|
@@ -54,15 +56,31 @@ export const EnvironmentCostEstimator = ({ | |
costTextColor = colors.accent, | ||
style, | ||
}: Props) => { | ||
const { detachedDisk, diskConfig } = analysisConfig; | ||
const { computeType, gpuConfig, machine, numNodes, dataprocConfig } = | ||
analysisConfig; | ||
|
||
// temp derive from analysisConfig | ||
const persistentDisk = derivePdFromAnalysisConfig(analysisConfig); | ||
|
||
const runningCostParams: RunningCost = { | ||
dataprocConfig, | ||
persistentDisk, | ||
computeType, | ||
gpuConfig, | ||
machine, | ||
numNodes, | ||
}; | ||
const runningCost = | ||
machineRunningCost(analysisConfig) + | ||
machineRunningCostPerHour(runningCostParams) + | ||
(isGKEApp ? GKE_APP_HOURLY_USD_COST_PER_WORKSPACE : 0); | ||
const runningCostBreakdown = machineRunningCostBreakdown(analysisConfig); | ||
const runningCostBreakdown = machineRunningCostBreakdown(runningCostParams); | ||
const pausedCost = | ||
machineStorageCost(analysisConfig) + | ||
machineStorageCostPerHour({ dataprocConfig, persistentDisk }) + | ||
(isGKEApp ? GKE_APP_HOURLY_USD_COST_PER_WORKSPACE : 0); | ||
const pausedCostBreakdown = machineStorageCostBreakdown(analysisConfig); | ||
const pausedCostBreakdown = machineStorageCostBreakdown({ | ||
dataprocConfig, | ||
persistentDisk, | ||
}); | ||
|
||
if (isGKEApp) { | ||
const gkeBaseCost = `${formatUsd( | ||
|
@@ -76,11 +94,9 @@ export const EnvironmentCostEstimator = ({ | |
...styles.cost, | ||
color: costTextColor, | ||
}; | ||
const pdCost = cond( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a good illustration of the complexity around how AnalysisConfig represents Persistent Disks.
My intention is to move the distinction between "detached" vs. otherwise to "persistent" vs. otherwise, which better reflects how we think about them today. see the new derivePdFromAnalysisConfig() which captures this logic. |
||
[diskConfig.detachable, () => diskConfigPricePerMonth(diskConfig)], | ||
[!!detachedDisk, () => detachableDiskPricePerMonth(detachedDisk)], | ||
() => 0 | ||
); | ||
const pdCost = persistentDisk | ||
? persistentDiskPricePerMonth(persistentDisk) | ||
: 0; | ||
return ( | ||
<FlexRow {...{ style }}> | ||
<FlexColumn style={styles.costSection}> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -346,9 +346,15 @@ export const CustomizePanel = ({ | |
gcePersistentDisk | ||
); | ||
|
||
const dataprocConfig = analysisConfig.dataprocConfig && { | ||
...analysisConfig.dataprocConfig, | ||
masterDiskSize: diskConfig.size, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it was always correct to set this, but it was not needed because it's also captured in |
||
}; | ||
|
||
setAnalysisConfig({ | ||
...analysisConfig, | ||
diskConfig, | ||
dataprocConfig, | ||
detachedDisk: diskConfig.detachable ? null : gcePersistentDisk, | ||
}); | ||
}} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -813,19 +813,11 @@ describe(withAnalysisConfigDefaults.name, () => { | |
existingDiskName: null, | ||
}; | ||
|
||
// yes it removes 3 fields. why? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed this discrepancy |
||
const expectedDataprocConfig: DataprocConfig = { | ||
...inputConfig.dataprocConfig, | ||
masterMachineType: undefined, | ||
masterDiskSize: undefined, | ||
numberOfWorkerLocalSSDs: undefined, | ||
}; | ||
|
||
const outConfig = withAnalysisConfigDefaults(inputConfig, inputDisk); | ||
|
||
expect(outConfig.computeType).toEqual(ComputeType.Dataproc); | ||
expect(outConfig.diskConfig).toEqual(expectedDiskConfig); | ||
expect(outConfig.dataprocConfig).toEqual(expectedDataprocConfig); | ||
expect(outConfig.dataprocConfig).toEqual(inputConfig.dataprocConfig); | ||
expect(outConfig.gpuConfig).toBeNull(); | ||
expect(outConfig.detachedDisk).toEqual(inputDisk); | ||
|
||
|
@@ -839,48 +831,19 @@ describe(withAnalysisConfigDefaults.name, () => { | |
); | ||
}); | ||
|
||
const replaceableFields = [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. all fields are now "replaceable" so we don't need this special logic |
||
'numberOfWorkers', | ||
'workerMachineType', | ||
'workerDiskSize', | ||
'numberOfPreemptibleWorkers', | ||
]; | ||
it('replaces the replaceableFields with their defaults when dataprocConfig is missing', () => { | ||
it('sets dataprocConfig to the preset default when it is missing', () => { | ||
const inputConfig = { | ||
...defaultAnalysisConfig, | ||
computeType: ComputeType.Dataproc, | ||
dataprocConfig: undefined, | ||
}; | ||
|
||
const outConfig = withAnalysisConfigDefaults(inputConfig, undefined); | ||
|
||
replaceableFields.forEach((field: string) => | ||
expect(outConfig.dataprocConfig[field]).toEqual( | ||
runtimePresets().hailAnalysis.runtimeTemplate.dataprocConfig[field] | ||
) | ||
expect(outConfig.dataprocConfig).toEqual( | ||
runtimePresets().hailAnalysis.runtimeTemplate.dataprocConfig | ||
); | ||
}); | ||
|
||
test.each(replaceableFields)( | ||
"it replaces %s with the default when it's missing", | ||
(field: string) => { | ||
const inputConfig = { | ||
...defaultAnalysisConfig, | ||
computeType: ComputeType.Dataproc, | ||
dataprocConfig: { | ||
...defaultAnalysisConfig.dataprocConfig, | ||
[field]: undefined, | ||
}, | ||
}; | ||
|
||
const outConfig = withAnalysisConfigDefaults(inputConfig, undefined); | ||
|
||
expect(outConfig.dataprocConfig[field]).toEqual( | ||
runtimePresets().hailAnalysis.runtimeTemplate.dataprocConfig[field] | ||
); | ||
} | ||
); | ||
|
||
// same as Standard VM | ||
it("should replace a missing diskConfig size with the persistent disk's when it exists", () => { | ||
const inputConfig = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -184,9 +184,15 @@ export const withAnalysisConfigDefaults = ( | |
dataprocConfig = { | ||
numberOfWorkers: | ||
dataprocConfig?.numberOfWorkers ?? defaults.numberOfWorkers, | ||
masterMachineType: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. previously, this did not set defaults for some fields. There was no clear reason for skipping these, and we do have defaults we can use for all of them. By adding them, we can simplify some logic. |
||
dataprocConfig?.masterMachineType ?? defaults.masterMachineType, | ||
masterDiskSize: dataprocConfig?.masterDiskSize ?? defaults.masterDiskSize, | ||
workerMachineType: | ||
dataprocConfig?.workerMachineType ?? defaults.workerMachineType, | ||
workerDiskSize: dataprocConfig?.workerDiskSize ?? defaults.workerDiskSize, | ||
numberOfWorkerLocalSSDs: | ||
dataprocConfig?.numberOfWorkerLocalSSDs ?? | ||
defaults.numberOfWorkerLocalSSDs, | ||
numberOfPreemptibleWorkers: | ||
dataprocConfig?.numberOfPreemptibleWorkers ?? | ||
defaults.numberOfPreemptibleWorkers, | ||
|
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.
my eventual goal is to stop using AnalysisConfig, but that's going to take some time