Add OG-Core cases, parameters, and run management - #522
Conversation
Cancelling a queued run only dropped it from the queue, so it stayed pending forever: nothing revisits a pending run, only a running one is repaired on a status read. It now gets the same terminal state a cancelled active run gets. Result tables named the base slot "Baseline" whichever run was in it, so viewing a reform on its own called it a baseline. OG-Core labels the two slots by position and takes no label argument, so a one-run table is relabelled with the run name on our side. Comparisons still read baseline/reform. Applies to the macro, inequality, gini and time series tables, and to the CSV download. Also stops an unwritable run_meta from breaking the queue drain and stranding everything behind it, and adds pytest coverage for the queue, cancel, labelling, run guards and restart recovery.
The parameter form came back empty for every country calibration: 129 of 133 parameters had no title, description, default or range. A country defaults file lists plain values while the base file carries the metadata, and the overlay projected both the same way, so it replaced everything it touched. The overlay now keeps the base metadata and swaps in the country's own value, which is also the value the user actually has. Deleting a baseline removes the whole case, but unlike deleteCase it asked for no session, so it was an unguarded case delete. It now clears the same gate; deleting a reform is unchanged. Parameters could also be changed while a run was already running or queued. The worker reads them when it launches, so that either left a finished run's saved parameters disagreeing with its results, or let a queued run slip past the dimension check it had already passed. saveParams and uploadTaxParams now refuse until the run finishes. Also: reforms record their baseline by name and resolve the path at launch, so a case restored on another machine still finds it; run_meta is written atomically since the status endpoints poll it, and one unreadable meta no longer empties the run list; restoreCase caps the upload and what it may expand into; run ids are no longer reused after a delete; and the reform dimension check now includes consumption goods alongside S, T, J and M.
# Conflicts: # API/app.py
Installing or updating a calibration rewrites the venv a solve is running under, so the two now refuse to overlap: a run will not start while an install is in flight for that country, and installing, updating or re-registering a calibration is refused while a run is using it. A first install is unaffected, since a country with nothing installed cannot have a run. The worker was spawned without -u. Writing to a pipe, Python block-buffers stdout, so a solve's progress sat in the child until it exited: an hour-long run showed one log line and never reported an iteration. It is spawned unbuffered now. A run left behind by a crash is repaired at startup as well as on the next status read, and its worker is killed if it outlived the server. The kill only fires when the recorded pid is still that run's worker, so a reused pid is never touched, and a run directory no longer matches one it is a prefix of. Stopping the server stops a running solve alongside a running install. Also: getRunStatus reports why a run failed rather than only that it did; a failed status read no longer overwrites a run that finished in the meantime; cancel kills outside the lock so it cannot block status polls; the wealth moments table accepts the data moments OG-Core needs to build it at all; and runs get an optional inactivity ceiling, off by default because a healthy solve can be quiet for a long time.
A malformed MUIOGO_OGC_RUN_TIMEOUT_SECONDS raised at import and took the whole app down, while the inactivity value beside it fell back quietly. Both read through one helper now, so a typo in a tuning knob leaves the default in place. Also covers the repair that only fires after a re-read, so a solve that finishes while its status is being polled is not written off as failed.
Without -ww, ps clips the command to the terminal width, or to $COLUMNS when there is no terminal. The run directory sits at the end of a worker's command line, so it was being cut off and the orphan check could never match: on Linux and macOS a leftover worker would simply never be recognised. It surfaced as a test failure because pytest sets COLUMNS.
OG-Core builds this table from a "Data" column of survey moments, and leaves that column empty when none are given, so the frame cannot be built at all and the endpoint failed for every calibration. Hand it blanks and drop the column afterwards, which leaves the model's own numbers. A scalar broadcasts, so this does not depend on how many moments the table has. Passing real data moments still returns both columns.
OGResults.run_completed and OGRunner.alive were written during the build and never called from anywhere, including the tests. The run layer's own docstring still described a single wall-clock watchdog, from before the inactivity ceiling and the orphan kill were added. The rest is whitespace left by earlier edits, and one copy of a test helper that existed in two files. No behaviour changes.
Functional testing on a deployed backend turned up two small gaps in the analysis tables. time_series took no options through the API while the worker behind it accepts stationarized, so the option could not be reached from outside. Same route/worker mismatch as the macro table output_type and the wealth moments data_moments. Asking for the macro table with only a steady state solved was refused with the path of the pickle it could not find, which tells the caller nothing they can act on. It now says there are no transition path results and to run with the full time path, matching what getResults already said. Only the transition path case changed; a missing steady state or model_params still names the file, since neither is something a caller can fix and the path helps when diagnosing one.
restoreCase unpacked the backup straight into the final case folder. A restore that stopped partway, on a full disk or a name the filesystem refuses, left the files written so far sitting under the case name. That wreckage read as a real case, and it also blocked the obvious fix of simply trying again, because the name now existed. Unpack into a staging directory and publish with one rename, so a failed restore leaves nothing at all. Staging goes beside the cases directory rather than inside it: same filesystem, so publishing stays a rename, and a restore in flight is never visible to the two things that walk the cases directory, list_cases and the startup reconcile pass, both of which treat any directory there as a case. The move can still lose a race, since the earlier existence check is not a lock, so a failure there reports the case as already existing when it now does. Nothing is published in that case either. Tests cover the round trip, the refusal to overwrite, and the three things the old code got wrong: no half-case is published, the retry after a failure works, and nothing appears under cases while unpacking.
error9098x
added
the
Track: OG Onboarding
Case names were global, so "Baseline" could only exist once across every country. Creating it a second time either hit a confusing refusal about country_id or, if the client left country_id out, silently edited the other country's case. Cases now live at cases/<country_id>/<casename>. A case is identified by the pair, so country_id is required wherever an endpoint names one, and the session carries both halves. The directory a case sits in decides its country: genData is pinned to it on write, and the listing reads it from the path. Two things fall out of that. is_country_running answers from the run's own key instead of reading genData, so an unreadable case cannot hide a live run from the install guard. And an edit can no longer move a case between countries, which retires the country_id immutability check. Cases stored in the old layout are moved under their country at startup. One that records no country is left alone rather than guessed at.
# Conflicts: # API/Classes/OGCore/OGCoreCase.py # API/Classes/OGCore/RunJob.py # API/Routes/OGCore/OGCoreRunRoute.py # tests/ogcore/test_run_queue_cancel.py
|
Fixed the legacy storage edge case: CLEWS now lists only directories containing genData.json, so stale WebAPP/DataStorage/OGCore state is no longer shown as a CLEWS model. No data is moved or deleted, and regression coverage was added in commit #cc5494d |
autibet
left a comment
autibet
left a comment
There was a problem hiding this comment.
The shape of this is good: the workspace lifecycle, the page-token guards against stale repaints, and the Tabulator-based table editor all land cleanly, and the new backend tests cover the queue and freshness rules well.
Three blocking issues, then smaller correctness, performance, and cleanup.
Blocking
1. A parameter edit makes a completed run read "Not run", with no reason shown
OGCoreCase.invalidate_run sets an invalidated run back to status="pending", clears completed_at, and records why in stale_reason. But the Run page decides staleness from the status:
// OGRuns.js:96
let backendStale = hasReusable
? (run.status == 'completed' && !run.reusable)
: ...For an invalidated run status is "pending", so backendStale is false, normaliseState returns 'pending', and the row renders "Not run". stale_reason is never read anywhere — git grep stale_reason WebAPP/ returns nothing.
Repro: complete a baseline → edit one parameter → open Run. The row that had results a moment ago says "Not run", explains nothing, and disappears from "Latest outcomes". The stale / "Needs run" state that STATUS defines is only ever reached by the calibration-commit-change path, which keeps status="completed" and flips reusable.
Proposed fix: make reusable: false the single invalidation signal and stop rewriting the status — drop meta["status"] = "pending" and the completed_at clear from invalidate_run, so a run that completed stays completed with reusable: false and a stale_reason. The existing run.status == 'completed' && !run.reusable test then fires unchanged, and _results_gate can gate on reusable instead of status so stale results still aren't served. Then render stale_reason in the row title or the outcome panel. If keeping the status rewrite is preferred for other reasons, the minimum is backendStale = hasReusable ? (!run.reusable && !!run.stale_reason) : ... plus surfacing the reason.
2. A failed createRun leaves an invisible case that can't be deleted and blocks its name
Baseline creation is two calls:
// OGCases.js:411
request = Ogc.saveCase({ casename: name, ... })
.then(response => OGCases.requireCreated(response, '...'))
.then(() => Ogc.createRun({ ..., run_name: 'baseline', run_type: 'baseline', ... }));If the second fails — dropped connection, a 400 from createRun — the case is already on disk with zero runs, and there is no way to get rid of it:
OGCases.entries()(OGCases.js:182) builds rows by iteratingc.runs, so a run-less case produces no row at all.data-act="del-case"is never emitted byentryRowsordefaultRow— onlydel-runis — soopenDeleteCase,deleteCaseConfirm, and thedel-case/del-case-confirmhandlers (OGCases.js:623,:651) are unreachable. There is no UI path to delete a case, orphaned or not.findCase(name)still matches it, so retrying the same name reports "A baseline with this name already exists."
Proposed fix: two parts.
- Make creation atomic. Either have
saveCasecreate the baseline run in the same request, or add a compensatingOgc.deleteCase(countryId, name)in the.catchofnewCaseConfirmbefore the error is shown. - Decide whether case deletion is meant to be reachable. If yes, add a
del-casecontrol to the baseline row's action menu — the dialog and handlers already exist, they just have no trigger. If no, deleteopenDeleteCase,deleteCaseConfirm, and both handler lines rather than leaving them as dead paths.
Guarding rendering against run-less cases (entries() emitting a bare case row) would also stop them being invisible in the first place.
3. Reloading outside the workspace strands the backend country session
Routes.Class.js:299 only calls OGWorkspace.leave() on a workspace→non-workspace transition. On a fresh page load acceptedHash equals the current hash, so currentRoute == route and nothing runs.
Repro: open the ETH workspace, then reload the browser at #/OGCore (or paste that URL). Both localStorage['osy-ogc-country'] and the backend's session['ogccountry'] survive. Click "Open workspace" on a different country: Ogc.setSession(null, 'ZAF') gets a 409, "Exit the active country workspace before opening another." The failure dialog offers only Retry (same 409) and "Return to OG-Core". The only escape is reopening ETH and exiting it through the confirm flow.
Proposed fix: reconcile once at startup, before the first crossroads.parse. If the entry route is not a workspace route but OGWorkspace.current() is set, call OGWorkspace.clearLocal() and Ogc.setSession(null) — no confirm dialog needed, since the user did not navigate away from anything. Belt-and-braces: let prepare() treat a 409 from setSession as recoverable by clearing the session and retrying once, so the dialog is never a dead end.
Smaller correctness
-
OGParameters.isCurrentis missing the page check —OGParameters.js:33is justpageID == PAGE_ID, andPAGE_IDonly changes inonLoad. Leave Parameters before its four-requestPromise.allresolves and the callback still renders into a destroyed DOM, then callsinitEvents(), which re-armsNavigationGuard.activate(...)and abeforeunloadlistener for a page you're no longer on — so the next navigation is arbitrated by the dead page's guard.
Fix: match the siblings —return pageID == PAGE_ID && localStorage.getItem('osy-pageId') == 'OGParameters';(OGCases.js:126,OGRuns.js:57already do this). -
The Run page stops polling for good if nothing is active at load —
OGRuns.js:616. The monitor's loop body opens withif (!active.length) return;, andload()— the only restart — runs just fromonLoadand therunSelectedfinally-block. A run started from another tab, or a FIFO entry this page didn't submit, never appears; rows stay frozen at their load-time state.
Fix: don't exit on an empty pass —awaitthe 2s delay and re-check, keeping the loop alive as long asisCurrent(pageToken)holds. Cheaper still: pollgetRunQueueper case on that tick instead ofgetRunStatusper entry, so an idle page costs one request per interval. -
_preceding_baseline_time_path_lockedconflates two cases —RunJob.py:279returnsmeta.get("time_path"), andget_run_metareturns{}for a missing or unreadable file, soNonemeans both "no baseline in flight" and "baseline in flight,time_pathunknown". The caller'spreceding_time_path is not Nonethen rejects a reform with "The baseline must complete before running a reform." while that baseline is actively running.
Fix: return(found: bool, time_path)and have the caller testfoundfor precedence andtime_path is Truefor the transition-path requirement. -
A successful migration is logged as a failure —
OGCoreCase.py:240. The order isos.replace(case_dir, staged)→os.replace(staged, target)→staged.parent.rmdir()→moved += 1. If thermdirraises (stray file, open handle on Windows), theexcept OSErrorbranch runs after the case is already correctly in place. No data is lost — the rollback guard correctly seesstagedis gone — butmovedisn't incremented and the log says "Could not move case", so an operator believes a case was left behind.
Fix: movemoved += 1and the info log to immediately after the secondos.replace, and wrap thestaged.parent.rmdir()in its own best-efforttry/except OSError: pass, the same waystaging_rootis already cleaned up.
Performance
-
Reusability fingerprinting is recomputed from scratch on every read —
OGCoreCase.py:460.get_runs_enrichedcallsis_run_reusableper run →execution_input_fingerprint→_calibration_identity→CalibrationRegistry.get, which reopens andjson.loadsthe registry file under a lock on every call (CalibrationRegistry.py:75), plus a params read and a full SHA-256 ofogcTaxParams.pklwhen present. ThegetRunsroute also callsRunJob.get_liveonce per run, each takingRunJob._lock. That per-run cost is fanned out over every case in the country byOGWorkspace.prepare,OGCases.refresh, andOGRuns.load— a dozen cases means dozens of registry parses and pickle hashes per page load.
Fix: three cheap changes, in order of payoff. (a) Resolve the calibration record once per request and thread it intoexecution_input_fingerprint, instead of callingCalibrationRegistry.getper run. (b) Cache the tax-params digest keyed on(path, st_mtime_ns, st_size)so an unchanged pickle is hashed once. (c) Take oneRunJobqueue snapshot pergetRunscall and match run names against it, rather than callingget_liveper run. -
Every pending run gets an extra status probe even when the snapshot succeeded —
OGRuns.js:219. The comment frames this as a fallback for older backends, butcandidatesselects everypendingor active entry and probes it unconditionally, including whengetRunQueuealready reported exactly which runs are queued or running. Each probe triggers a server-side fingerprint recomputation, so 30 configured-but-unrun runs means 30 extra round trips per Run-page load.
Fix: track whether everygetRunQueuecall succeeded and skip the probe loop entirely when they did; probe only the cases whose snapshot request failed. Thetypeof Ogc.getRunQueue == 'function'check on line 208 — for a method this same PR adds — can go at the same time.
Cleanup
-
Three parallel answers to "can this result be reused?" — worth a decision rather than a patch. Reusability is currently computed by (a)
input_fingerprint/result_fingerprint+is_run_reusable, (b)invalidate_runoverwritingstatustopending, and (c)markRunsStalein browser localStorage — then re-derived a fourth time inOGRuns.buildQueue.canReuseand again inrender. They already disagree: (b) fires first on the common path and masks (a), which is what produces the "Not run" bug, and it also makes (c) unreachable.
Fix: keep (a) as the only source of truth — the backend owns the verdict and reportsreusable+stale_reasonon bothgetRunsandgetRunStatus; delete (b)'s status rewrite and (c) entirely; and have the frontend readentry.reusablein one place rather than re-deriving it incanReuseandrender. -
The localStorage staleness store is write-only —
OGCases.js:20.isRunStaleis read at exactly one site,OGRuns.js:98, inside therun.reusable === undefinedbranch — which this PR's backend never produces, sinceget_runs_enrichedsetsreusableon every run. MeanwhileOGParameters.save()(OGParameters.js:954) still callsmarkRunsStaleon every save, soosy-ogc-stale-runsaccumulates an entry per run forever and is never read back.
Fix: deleteSTALE_KEY,loadStale,markRunsStale,isRunStale,clearRunStale, themarkRunsStalecall insave(), and thereusable === undefinedfallback branch; add a one-linelocalStorage.removeItem('osy-ogc-stale-runs')on workspace entry so existing installs shed the orphaned key. -
escis copy-pasted into five files —OGCases.js:9,OGCore.js:11,OGParameters.js:9,OGRuns.js:8,OGWorkspace.Class.js:17, all verbatim. It's the escaper guarding every interpolated backend value, so a fix to one silently misses four.
Fix: export it once (e.g.WebAPP/Classes/Html.Class.jsor alongsideMessage.Class.js) and import it in all five. -
OGTableEditorre-implements four helpers, and one has already drifted —clone,equal,flatten,dimensions(OGTableEditor.js:1,:3,:19,:26) duplicateModel.clone/Model.equal(OGParameters.Model.js:281,:305) andOGParameters.flatten/.dimensions.OGTableEditor.equalrunsparseFloatfirst whileModel.equalshort-circuits null/undefined before the numeric compare, soequal(null, 0)is false in the model and true in the table editor — the grid and the field view can disagree about whether a cell changed.
Fix: importModel.clone/Model.equalinOGTableEditorand delete its local copies; moveflatten/dimensionsontoModel(or a small shared array-helpers module) so the parameters page and the grid share one definition of "changed".
|
@error9098x Add "hover text" when you're over buttons for more than 1 second with a brief explanation of what the button does: I'd remove "Change country". They can already do that just by pressing on the Home icon so it adds nothing. Shouldn't all buttons be orange? Or is there a reason why "Add Case" is the only orange? "Separate Tables" is quite meaningless — it needs a better name to easily understand what it's for. @marcelolafleur Shouldn't the "Default calibration" already be an inmutable Baseline in itself? Do we need to create a Baseline just to be able to run the model? |
|
@error9098x Can the log window automatically scroll down so the user can see the latest messages without having to scroll down constantly? Every time it updates it sends you back to the top: Shouldn't the |
3 participants
Summary
What changed:
editing, and run selection.
cached-result status, and rerun handling.
without changing CLEWS behavior.
recovery, navigation, and workspace lifecycle.
Why:
calibration has been opened: users can create a baseline, add
reforms, adjust parameters, and run the model.
Parameter tables
The Parameters page uses Tabulator for structured, editable parameter tables. It provides the required table editing,
validation, sorting, and navigation behavior without introducing a custom grid implementation.
Tabulator is open source under the MIT License. Version 6.5.0 and its license are vendored in the
repository.
Linked issue
Screenshots
Ethiopia Cases workspace
Shows the country workspace, baseline/reform organisation, case
actions, and country context.
Parameters Page
Active model run
Shows a running reform with current status, live worker log output,
and recent outcomes.
Validation
recovery, navigation, and country-access behavior.
completed.
Checklist
the required workspace lifecycle.
main; rebaseafter Standalone OG-CORE Ingestion Pipeline Backend #498 merges. PR target:
EAPD-DRB/MUIOGO:main.Core workspace/run layer before this draft is marked ready.