-
Notifications
You must be signed in to change notification settings - Fork 1
Store household calculation outputs in Simulation records instead of Report #235
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
d42b8b5
to
89e7e66
Compare
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.
Thanks for this, Sakshi. TL;DR: I'd move the executeCalculation
code to the household-level handler instead.
// Simulation IDs needed for storing household calculation outputs | ||
simulationIds: [ | ||
String(report.simulation_1_id), | ||
...(report.simulation_2_id ? [String(report.simulation_2_id)] : []), |
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.
nit: Make this less hacky; I was about to write an issue asking if this didn't nest sim 2's ID inside an array, inside another array, until I re-read the manipulation code
app/src/libs/calculations/service.ts
Outdated
for (let index = 0; index < meta.simulationIds.length; index++) { | ||
const simulationId = meta.simulationIds[index]; | ||
const policyId = index === 0 ? meta.policyIds.baseline : meta.policyIds.reform; | ||
|
||
if (!policyId) { | ||
continue; | ||
} | ||
|
||
const singleSimMeta: CalculationMeta = { | ||
...meta, | ||
policyIds: { baseline: policyId, reform: undefined }, | ||
simulationIds: [simulationId], | ||
}; | ||
|
||
await this.householdHandler.execute( | ||
`${reportId}-sim-${simulationId}`, | ||
singleSimMeta, |
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.
issue, blocking: I'd highly recommend modifying the household handler instead.
The aim of executeCalculation
is to occur at the report level, executing an entire report's worth of calculation. In the long-run, with API v2, ideally we'd just move the functionality to the simulation level, which would make this easier. For the time being, with households having a sim-level calculation, it'd be more effective to add the onSimulationComplete
function to the handler itself and return the calculation service code back to what it was, I think.
f979b9b
to
d7933cd
Compare
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.
@SakshiKekre left some commentary on this.
const { data: simulationData } = useQuery({ | ||
queryKey: ['simulation', reformSimId || 'none'], | ||
queryFn: () => fetchSimulationById(metadata!.countryId, reformSimId!), | ||
enabled: !!baseReportId && outputType === 'household' && !!reformSimId, |
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.
question: Are you doing only the reform because in a next PR, you'll add the code for the comparison?
if (parsed.countryId && parsed.householdData) { | ||
// Already a Household object, just add the ID | ||
output = { | ||
...parsed, | ||
id: baseReportId, | ||
}; | ||
} else { | ||
// Raw HouseholdData, need to wrap it | ||
const wrappedOutput: Household = { | ||
id: baseReportId, | ||
countryId: metadata!.countryId, | ||
householdData: parsed, | ||
}; | ||
output = wrappedOutput; | ||
} |
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.
question: Why is this necessary?
const allCompleted = simulations.every((sim) => sim.completed); | ||
const anyError = simulations.some((sim) => sim.error); | ||
|
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.
question: If a user runs, say, 4 household calculations within 5 minutes, won't this require all 4 to be complete to show any household report as complete?
meta.type === 'household' ? onComplete : undefined | ||
); | ||
// Execute calculation with callbacks | ||
const result = await this.service.executeCalculation(reportId, meta, callbacks); |
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.
question: Are you sure this and the above section don't break society-wide calculations?
Fixes #226
Problem
Household calculation outputs were incorrectly stored in Report.output due to economy calculation limitations being applied to household calculations. Ideally, each Simulation should store its own output.
Solution Overview
Implemented a callback-based flow where the Handler manages per-simulation execution and the Manager orchestrates storage via callbacks.
Architecture Changes
Added simulationIds to CalculationMeta
Why: The handler needs to know which simulations to run calculations for. Critical for the waterfall pattern (direct URL navigation/page refresh) where metadata must be reconstructed from the report.
Handler Executes Multiple Simulations
Why: Each simulation needs its own calculation with its own policy (baseline vs reform).
Manager Provides Storage Callbacks
Why: Separation of concerns - Handler handles calculation logic, Manager handles storage/side effects (dependency inversion).
Related Backend Changes (policyengine-api)
The Complete Flow
- Handler executes calculation → calls onSimulationComplete → Manager stores in Simulation
- Handler calls onComplete → Manager stores Report with output={}