[risk=low][no ticket] Logic changes for runtime cost display #8980
Conversation
| @@ -76,11 +94,9 @@ export const EnvironmentCostEstimator = ({ | |||
| ...styles.cost, | |||
| color: costTextColor, | |||
| }; | |||
| const pdCost = cond( | |||
There was a problem hiding this comment.
this is a good illustration of the complexity around how AnalysisConfig represents Persistent Disks.
- if the
diskConfigindicates that the disk is detachable, then thediskConfigrepresents a PD - else: the
diskConfigrepresents a standard (non-persistent, non-detachable) disk - there may also be a
detachedDisk. If so, that one is a persistent disk
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.
| const { computeType, gpuConfig, machine, numNodes, dataprocConfig } = | ||
| analysisConfig; | ||
|
|
||
| // temp derive from analysisConfig |
There was a problem hiding this comment.
my eventual goal is to stop using AnalysisConfig, but that's going to take some time
| export const diskPrice = diskPricePerMonth / approxHoursPerMonth; // per GB hour, from https://cloud.google.com/compute/pricing | ||
| export const ssdPricePerMonth = 0.17; // per GB month | ||
| export const dataprocCpuPrice = 0.01; // dataproc costs $0.01 per cpu per hour | ||
| const standardDiskPricePerMonth = 0.04; // per GB month, from https://cloud.google.com/compute/pricing |
There was a problem hiding this comment.
a lot of renaming and rearranging here to better explain what these values mean and what the functions are doing
|
|
||
| const dataprocSurcharge = ({ | ||
| interface DataprocSurcharge { |
There was a problem hiding this comment.
be more explicit about what these functions need
| return fp.sum([ | ||
| diskConfigPrice(diskConfig), |
There was a problem hiding this comment.
These two lines cover the same costs but in different ways.
before:
- L433 was the cost of the "main disk" of the runtime: the GCE PD or the DataProc Master
- L434 was the cost of any detached PD if one exists
after:
- L466 is the cost of the PD, whether attached (to GCE) or not
- L467 is the cost of the DataProc Master node disk, if it exists
| @@ -184,9 +184,15 @@ export const withAnalysisConfigDefaults = ( | |||
| dataprocConfig = { | |||
| numberOfWorkers: | |||
| dataprocConfig?.numberOfWorkers ?? defaults.numberOfWorkers, | |||
| masterMachineType: | |||
There was a problem hiding this comment.
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.
| @@ -813,19 +813,11 @@ describe(withAnalysisConfigDefaults.name, () => { | |||
| existingDiskName: null, | |||
| }; | |||
|
|
|||
| // yes it removes 3 fields. why? | |||
There was a problem hiding this comment.
removed this discrepancy
| @@ -839,48 +831,19 @@ describe(withAnalysisConfigDefaults.name, () => { | |||
| ); | |||
| }); | |||
|
|
|||
| const replaceableFields = [ | |||
There was a problem hiding this comment.
all fields are now "replaceable" so we don't need this special logic
| @@ -346,9 +346,15 @@ export const CustomizePanel = ({ | |||
| gcePersistentDisk | |||
| ); | |||
|
|
|||
| const dataprocConfig = analysisConfig.dataprocConfig && { | |||
| ...analysisConfig.dataprocConfig, | |||
| masterDiskSize: diskConfig.size, | |||
There was a problem hiding this comment.
it was always correct to set this, but it was not needed because it's also captured in diskConfig. Now we need this because we use this value to calculate costs.
jmthibault79
changed the title
Attempt to simplify the runtime UI code and logic a bit - part 1 of many.
Rename "detached" disk references to "persistent" which better reflects our use
Make arguments to cost functions more explicit about what they need
Reduce usage of AnalysisConfig and DiskConfig
Tested locally
PR checklist
[risk=no|low|moderate|severe]in the PR title as outlined in CONTRIBUTING.md.