fix(web): repair the web interface — startup, results, progress and privacy - #1
Open
42piratas wants to merge 15 commits into
Open
fix(web): repair the web interface — startup, results, progress and privacy#142piratas wants to merge 15 commits into
42piratas wants to merge 15 commits into
Conversation
The web interface could not start, and would not have served a request if
it had. Six defects, all on the `allelio serve` path — the CLI is unaffected.
- `starlette.concurrency.run_in_executor` does not exist and never has.
The import aborted `allelio serve` before uvicorn bound a port. The name
is unused in the module; the code calls `loop.run_in_executor`.
- `TemplateResponse(name, {"request": request})` is the removed positional
form. `/` returned 500.
- `AIEngine.check_connection` is async and was called without `await` in two
places. `/api/status` then tried to serialize a coroutine and returned 500;
the upload path treated a truthy coroutine as a live connection.
- `AllelioDB.get_statistics` is not a method — it is `get_stats`. The
AttributeError was swallowed, so the UI reported 0 ClinVar and 0 GWAS
entries against a fully populated database.
- `get_variant_warnings` takes one argument and was passed two, failing
every upload after the analysis had already run.
- The sqlite connection was opened on the event loop thread and then used
from `run_in_executor`, which sqlite refuses by default.
The template also read `database_ready` and `ollama_connected`, while
/api/status returns `db_ready` and `ollama_available`, so the status panel
read "Database Not Set Up" whatever the real state was.
Verified against a 630,774-variant 23andMe export: `allelio serve` starts,
`/` renders, `/api/status` reports 2,645,423 ClinVar and 1,191,572 GWAS
entries, and `POST /api/analyze` returns 62,057 results.
42piratas
marked this pull request as draft
August 27, 2026 23:14
Author
|
Moving this to draft — I found two more defects of the same class in
Both were caught by Will push both and mark ready once I have a full run through the web UI confirming the explanations actually render. |
Uploading a genome through the web UI looked like it hung. The progress
bar animated to 90% in fifteen seconds and then stopped, while the run
itself takes about ten minutes on a whole genome. Nothing told the user
where it had got to.
- Add /api/progress and have the page poll it, so the bar reports the
actual stage and count ("Writing explanations - 27 of 50").
- Run the explanations through explain_variants_batch, which was already
in the codebase but unused by the web route. 718s -> 569s.
- Call explain_variant, the method that exists; the route called
generate_explanation and every explanation came back as a failure
string.
- generate_summary treated ClinVar/GWAS entries as dicts, and sent the
model counts without the variants, so it replied that the list "was not
included in your message". Pass the findings.
- Cap the results list at 200 rows. Drawing all 62,057 locks the browser.
- Keep the last run in ~/.allelio/last_analysis.json and restore it on
load, with a button back to the upload panel.
…ests allow_origins=["*"] with allow_credentials meant any page the user happened to have open could POST their genotype file to localhost and read the analysis back. The UI is served from this same app; only a dev server on another loopback port needs CORS at all. Adds tests/test_web.py: the routes answer, a foreign origin gets no access-control-allow-origin header, and the two AI entry points are exercised against a stubbed client — including the assertion that the summary prompt actually carries the variants.
… AI timeout The results list reads result.gene, result.significance and result.pubmed_id. The route sent clinvar_data and gwas_data, which are not fields on VariantResult, so every card in a 62,000-variant report rendered as "Gene: Unknown" with a BENIGN badge — including the pathogenic ones. Sixty seconds also turned out to be short. On llama3.1:8b, the model this project defaults to, 28 of 50 explanations came back as "Request timed out". At 300s none do. Verified end to end in a browser against Ollama: first card now reads rs80224560 / CFTR / PATHOGENIC, 200 of 200 cards name a gene, 50 of 50 explanations complete.
The tabs filtered on slugs (health_conditions), the analyser labels results "Health Conditions". Four of the five tabs showed "No results found for this category" on every report.
Dropping the 200-row slice added while chasing the browser freeze. It does not reproduce: all 62,057 findings render in under four seconds. Paging a list this size is worth doing, but it is not part of this fix.
Follow-up on review of this branch. Security: - Result cards built the DOM by string interpolation from the uploaded file and the model. Nothing validates a genotype except a "--" check, so a crafted file executed script in the page. Escape every field, and only accept digits for the ClinVar and PubMed IDs that go into a URL. - The exported HTML report had the same hole, server side. - Removed the CORS middleware outright. The UI is same-origin with the API, so no origin needs to be granted anything; a loopback allowlist still let any local process read a genome off the API. Correctness: - "Conflicting interpretations of pathogenicity" was badged PATHOGENIC and pushed into the summary prompt as high impact, on a substring match. - explain_variants_batch reported progress twice per variant, once with a stale count, so the bar counted backwards. - The summary prompt read .gene on a GWAS entry, which names it mapped_gene. - The top-50 sort keyed on significance_score, an attribute no result has; analyze_variants already orders them. - The category tabs omitted carrier_status, which the analyser emits. - pyproject allowed fastapi 0.104, whose starlette predates the TemplateResponse(request, name) signature this branch now uses. Scope: - Dropped the last-analysis file and its endpoint. Writing a whole genome's findings to disk and replaying them for whoever next opens the page is a feature with a privacy cost, not a crash fix; it belongs in its own PR.
Picking a tab looked the button up by its position in a second, hardcoded category list. Anything missing from that list indexed to -1 and threw before the results were re-rendered, so the tab appeared to do nothing. The tabs now carry their own category. That list was also missing two of the six categories the analyser emits: carrier status and uncategorized. On a real genome that is 21% of the findings, reachable only under All.
Second pass over the same ground, after review. - The exported report escaped its rows but not its own two header fields, and /api/export takes whatever dict it is handed. - The summary's pathogenic test disagreed with the badge on the card: it had no benign guard, so a combined value could be badged benign in the UI and sent to the model as high impact. - _strong_gwas compared p-values by slicing the exponent out of the string form. p_value is a float column and 6,271 GWAS rows hold 0.0, which has no exponent at all, so the strongest associations in the catalogue were read as the weakest. Compare the number. - The ClinVar source link keyed on a clinvar_id that is in no payload and on no entry, so it rendered for no variant. Point it at the rsID. - _get_top_categories lost its only caller when the summary prompt changed; it was the last thing importing List and VariantResult here.
…inks The summary and the per-variant explanations are the one thing the AI is here for, and both arrived as a single run-on block: the model writes paragraphs, HTML collapses the newlines. pre-wrap on the two elements that hold prose. Also rel="noopener noreferrer" on the ClinVar and PubMed links.
…ean up the export ClinVar's "Conflicting classifications of pathogenicity" is 130,833 of the rsIDs in the shipped dump and sorts to the top of every report. It was drawn in the same green as a benign call. It now has its own badge, and so does a trait, which was also borrowing benign's green. A ClinVar benign call was unreachable for any variant that also had a GWAS row, because the GWAS check ran first. The safety layer computes a genetic-counselling warning for BRCA1/2, TP53, Lynch and APOE. Nothing rendered it. The cards and the exported report do now. Explanations were gated on the model being reachable, so a machine without Ollama got empty cards instead of the ClinVar/GWAS fallback that explain_variant already writes. The exported report was left in the shared temp directory at 0644 forever with the user's genotypes in it. It is now 0600 and deleted once sent.
…ging The multipart filename went straight into a temp path. "../../.zshenv" resolves out of the temp directory, the route writes the body there and then unlinks it, and multipart/form-data is CORS-safelisted, so a page on the open web could do it without a preflight. mkstemp now picks the name — and the mode, which matters more than it does for the report: this file is the user's entire genome and it was landing at 0644 in a shared directory. A GWAS row on its own was badged RISK. That is 37,108 of the 62,057 findings on a real genome, every one of them sitting under a tab the analyser labelled Traits. "Uncertain significance" is 1,236,063 ClinVar rows, the largest class, and it had no branch at all — so a variant nobody can interpret was drawn as a harmless trait. It has its own badge now. A protective call reads as benign, which is the honest colour for it. explain_variant already folds the counselling warning into the explanation, so the new warning box repeated it verbatim on exactly the top-50 variants a user actually reads. It now only fills the gap it was meant for. Fifty explanations, three at a time, each allowed 300s, can hold the upload open for over an hour. The batch has a 15-minute ceiling and keeps whatever finished.
…rning Last commit made "has an explanation" stand in for "the counselling warning is already inside that explanation". It is not: explain_variant only runs wrap_with_disclaimer on the path where the model answered. Every fallback — no Ollama, a timeout, any error — writes an explanation with no warning in it, so on the machines the README says are supported, the BRCA1 and Lynch warnings vanished from the card and the report entirely. Test the text. Last commit also traded over-badging for under-badging. Delegating a GWAS-only variant to the analyser's category sounds right until you read the test it does: a literal search for "risk" or "association" in the trait string, which 2,068 of the 553,549 GWAS rsIDs pass. Type 2 diabetes, coronary artery disease and prostate cancer all came out as a blue TRAIT. Neither guess is available, so the badge now says association, which is what a GWAS row is. One variant failing outside explain_variant's own guard used to cost one explanation; since the per-variant try/except moved out of the route it cost the whole upload, minutes after the analysis had finished. The progress bar kept the explanation phase's counts through the last stage, so a batch cut short by the new deadline left it reading "12 of 50" and frozen at 29% for the longest step remaining.
…ing where it shows Fifteen minutes was too tight a ceiling — a full run of this project's own default model takes longer than that on an M1 Max, so the cap would have cut a normal run short. Thirty. Worse, a variant the deadline cut off got an empty string, and an empty string renders as "No explanation available". _fallback_explanation writes the gene, the ClinVar call and the GWAS traits, and it only runs inside the coroutine that was cancelled — so a user with a slow but working Ollama ended up with strictly less than a user with none. The batch starts from the fallbacks and lets the finished ones overwrite. The counselling warning went back on the card in the last commit, into .result-body, which is display:none until the card is clicked. A BRCA1 card looked like every other card. It sits under the header now. The progress bar only moved during the explanation phase, so it read 10% through everything else — including a summary step that can now run for half an hour. The stages without a count carry their own width. The report's cells had no pre-wrap, so a newline-formatted fallback explanation collapsed into one run-on line in the artefact the user keeps.
42piratas
marked this pull request as ready for review
August 28, 2026 04:50
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
allelio serveexits on import, and every request path behind it has a defect of itsown. I found these trying the web UI on a real 23andMe export; the CLI path works and
is untouched here.
This started as the startup crash and grew as each fix exposed the next one. It is
fixes only — no new features — one commit per theme, so it reviews in pieces.
Startup and wiring
web/routes.pyfrom starlette.concurrency import run_in_executor— that name is not in starlette and is not in its history. The import abortedallelio servebefore uvicorn bound a port. It was also unused: the code callsloop.run_in_executor.web/routes.pyread_rootTemplateResponse("index.html", {"request": request})is the old positional form. On the starlette this resolves to today it raisesTypeError: unhashable type: 'dict', so/returned 500.web/routes.py×2AIEngine.check_connectionisasyncand was called withoutawait./api/statusreturned 500 serialising a coroutine; the upload path read a truthy coroutine as a live connection.web/routes.pyget_statusdb.get_statistics()— the method isget_stats. The AttributeError was swallowed by the surroundingexcept, so the UI reported 0 ClinVar and 0 GWAS entries against a full database.web/routes.pyanalyze_fileget_variant_warnings(rsid, genotype)— it takes one argument, theVariantResult. Every upload failed here, after the analysis had already run.database/store.pyrun_in_executor. sqlite refuses that by default.templates/index.htmldatabase_ready/ollama_connected;/api/statusreturnsdb_ready/ollama_available. The status panel said "Database Not Set Up" against a full database.pyproject.tomlfastapi>=0.104.0resolves a starlette that predates theTemplateResponse(request, name)signature. On a clean install at the declared floor the index page 500s. Floor raised to 0.108.The upload looked like it hung
It did not — a whole genome takes about twelve minutes. The progress bar was on a
timer, faked its way to 90% in fifteen seconds and stopped there, with no text. There
was nothing to distinguish that from a hang.
/api/analyzenow publishes its stage to/api/progressand the page polls it, sothe bar reflects the run: parsing, matching, then
n of 50while the explanationsare written.
explain_variants_batchinstead of a sequential loop(12 min → 9.5 min on this machine).
explain_variants_batchreported progress twice per variant, once with a stalecount, so the bar counted backwards. It reports once now.
model once a few explanations run at once — 28 of 50 came back as the timeout
fallback. 300s: 0 timeouts on the same input. That alone would let 50 variants,
three at a time, hold the upload open for over an hour, so the batch also gets a
30-minute ceiling and keeps whatever finished — as the fallback text, not as an
empty string, so a slow model never leaves the user with less than no model would.
explain_variant's own guard used to cost oneexplanation. Once the per-variant
try/exceptmoved out of the route it cost thewhole upload, minutes after the analysis had finished.
promises the tool works without it, so it now completes and says the summary is
unavailable.
The results list showed the wrong thing
clinvar_data/gwas_data, fields that do not exist onVariantResult. Every card read "Gene: Unknown" and every variant — pathogenicones included — was badged BENIGN. They now get
gene,significanceandpubmed_idoff the entries that are actually there."Conflicting classifications of pathogenicity"was badged PATHOGENIC on asubstring match — it contains the word. It is 130,833 rsIDs in the dump
allelio download-dbfetches, and it sorts to the top of the report, so it was the firstthing a user saw. Calling it BENIGN instead would be the same lie in green, so it
has its own badge now. A trait does too, rather than borrowing benign's green.
health_conditionswhile the analyser emitsHealth Conditions, so four of the five tabs said "No results found".list. Anything missing from that list indexed to
-1and threw before the resultsre-rendered.
and uncategorized — 21% of the findings on a real genome, reachable only under All.
clinvar_idthat is in no payload and on noentry, so it rendered for no variant. It points at the rsID now.
never been shown, so the model answered by saying so. It gets the variants.
_strong_gwascompared p-values by slicing the exponent out of the string form.p_valueis a float column and 6,271 GWAS rows hold0.0, which has no exponentat all, so the strongest associations in the catalogue were read as the weakest.
significance_score, an attribute no result object has.It was dead rather than wrong —
analyze_variantsalready returns them sorted —so it is gone.
because the GWAS check ran first. ClinVar has the last word now.
genome, every one of them under a tab the analyser itself labelled Traits. Calling
them all a trait instead demotes type 2 diabetes and coronary artery disease, and
the analyser cannot tell them apart either: its GWAS test is a substring search for
"risk" or "association" in the trait string, which 2,068 of the 553,549 GWAS rsIDs
pass. So the badge says ASSOCIATION, which is what a GWAS row is.
"Uncertain significance"had no branch at all, so it came out as a trait. It is1,236,063 ClinVar rows — the largest class — and it passes the benign filter, so a
variant nobody can interpret was being drawn as a harmless characteristic. It has
its own badge now, and a
protectivecall reads as benign.get_variant_warningscomputes a genetic-counselling warning for BRCA1/2, TP53,Lynch and APOE. It was computed for all 62,057 results and rendered nowhere. It is
on the card and in the exported report now — once, not twice:
explain_variantalready folds it into the explanation, but only on the path where the model
answered, so the test is on the text rather than on whether an explanation exists.
It sits under the card header rather than inside the collapsed body — a BRCA1 card
otherwise looked like every other card until you clicked it.
got empty cards — even though
explain_variantalready writes a ClinVar/GWASfallback for exactly that case.
explanation arrived as one run-on block. The exported report needed the same.
through every other stage.
Security
allow_origins=["*"]withallow_credentials=True. Any page the user visitedcould POST a genome to localhost and read the result back. The UI is same-origin
with the API, so the middleware is gone rather than narrowed.
from the uploaded file. Nothing validates a genotype except a
--check, so acrafted file executed script in the page. Every field is escaped now, and the two
values that land inside a URL are pattern-checked rather than escaped.
its own header fields — and
/api/exportaccepts an arbitrary dict.Path(tempfile.gettempdir()) / file.filenamewith../../..in it resolves out ofthe temp directory; the route writes the request body there and then unlinks it.
multipart/form-datais CORS-safelisted, so this needed no preflight and droppingthe CORS middleware does not close it.
mkstemppicks the name now.shared directory.
mkstempgives it 0600.with the user's genotypes in it. Same treatment, plus it is unlinked once sent.
rel="noopener noreferrer"on the outbound links.Verified
Against a 630,774-variant 23andMe export (Personal Genome Project, openly consented),
in Chrome, with Ollama and
llama3.1:8b:allelio servestarts,/renders,/api/statusreportsdb_ready: truewith2,645,423 ClinVar and 1,191,572 GWAS entries
analyser produces
<img onerror>payload renders asinert text, with no script execution
tests/test_web.pyis new and covers the routes, the CORS behaviour, the twoclassifiers, the escaping on both sides, the p-value comparison, the upload path and
the batch deadline. Full suite: 107 passing.
Left alone deliberately
_get_significance_rankinanalysis/lookup.pysubstring-matches"pathogenic"inside
"pathogenicity", soConflicting classifications of pathogenicityscoresthe same rank as a true pathogenic call and files under Health Conditions. On a real
genome that interleaves 5,048 conflicting variants with 7,959 genuine ones at the top
of the report. This PR gives them an honest badge; the ranking is an analysis-module
change that moves CLI output too, so it wants its own PR. (
SIGNIFICANCE_RANKSalready has a
conflicting interpretationskey at rank 6 — ClinVar retired thatwording, the dump says
classifications, so the key never matches.)_progressis a module global, so two concurrent runs clobber each other'sbar. Needs a per-run token; out of scope here.
AllelioDB.close()has no caller, so each request leaks a connection.the report's
results[:100], though that means the export carries 79 of the 1,634counselling warnings a real genome produces.
_strong_gwas'sp < 1e-5matches 1,186,123 of the 1,191,572 GWAS rows, so itfilters nothing. The old code had the same effective threshold, so this is not a
regression — but the number the summary prompt is given is inflated.
switch tabs — so it is not the freeze I first assumed, but paging would be kinder.
Happy to split any of this out or drop pieces you would rather not carry.