Skip to content

El banco de donantes no tenia orden, y el orden es parte del resultado - #1007

Merged
github-actions[bot] merged 1 commit into
developfrom
fix/el-banco-de-donantes-sin-orden
Oct 12, 2026
Merged

github-actions[bot] merged 1 commit into
developfrom
fix/el-banco-de-donantes-sin-orden

Conversation

@frapercan

Copy link
Copy Markdown
Owner

Por que el orden del banco es un resultado y no una limpieza

El 91% de las consultas tienen vecinos a distancia ~0, ellas mismas o gemelas con
la misma secuencia. Asi que los terminos candidatos empatan en el k-esimo
hueco y la busqueda admite al donante que vaya PRIMERO en el banco. El orden del
banco decide, por tanto, que pares se publican.

Dos sitios lo decidian por accidente.

1. La consulta del pool unificado no llevaba ORDER BY

El orden era el que emitiera el plan. Medido sobre el banco de la 219, 142.422
filas, las mismas filas en todos los casos:

plan digest del orden
hash join, sin paralelismo 89b79ad15383
hash join, work_mem bajo 89b79ad15383
merge join (enable_hashjoin=off) cf77c750bed5
las mismas filas ordenadas por accesion cf77c750bed5

El plan por si solo movia f_micro_w ~1e-3 por celda, concentrado en CCO y con
signos mezclados. Y el plan depende de estadisticas y de work_mem, que en esta
maquina es justo lo que se mueve.

2. El indice por aspecto se construia iterando un set de cadenas

Peor que lo anterior. CPython aleatoriza el hash de cadenas por proceso salvo que
PYTHONHASHSEED este fijado, y aqui no lo esta. Comprobado pasando las mismas
ocho accesiones por tres interpretes:

P12345,B2RXH2,Q4GWU5,A0A014NF26,O00142
P0DMV8,O00142,B2RXH2,Q4GWU5,Q9Y6K1
A0A014NF26,Q4GWU5,P12345,P0DMV8,A6NMY6

Asi que el orden del banco por aspecto cambiaba en cada arranque de worker que
reconstruyera la cache, no solo cuando cambiaba el plan. La ruta por aspecto es la
que usa la comparacion con el nodo de computo.

Extraido a _aspect_indices, que ordena por accesion. Se ordenan las accesiones y
no los indices resultantes, para que siga siendo correcto con independencia del
orden en que quede el pool unificado.

Lo que lo tapaba

La cache en disco congelaba un orden. Una repeticion identica del mismo payload
reproducia par a par: 187.834 pares, 0 solo en A, 0 solo en B, 0 con distancia
distinta
. Pero esa cache tiene ref_cache_freshness_seconds, asi que el
enunciado real no era "dos corridas pueden discrepar", era "una corrida de hoy
puede discrepar de una de la semana que viene, en silencio, por 1e-3 por celda".
Eso es lo que el sello de marco existe para impedir.

Esto tambien acota un hecho que ya teniamos medido: la evaluacion reproduce 117 de
117 metricas exactamente, asi que el suelo de reproducibilidad no vive en la
puntuacion. Vive en la recuperacion.

Por que accesion

Porque es el orden que una reproduccion externa elegiria sin que nadie se lo diga,
y porque alinea el desempate con el del nodo de computo, cuyo banco ya va por
accesion. El desacuerdo medido entre los dos arneses sobre 219->220, cuatro celdas
de nueve identicas y el resto dentro de 8e-4, deberia pasar a coincidencia exacta.

Tests

Diez nuevos en tests/test_the_donor_bank_has_an_order.py, y la suite de
prediccion entera en verde (221, uno saltado).

El pool: que ordena por accesion, que lo hace tambien con una politica de
donantes restringida y con reviewed_only, y que el ORDER BY es el ultimo y uno
solo, no dentro de la subconsulta de anotadas, donde no ordenaria nada util.

El indice por aspecto, de forma determinista: el colector se sustituye para
entregar las accesiones en orden INVERSO, asi que iterar la entrada tal cual
escribiria el reverso y solo ordenar escribe orden de accesion. No depende de que
un set real tenga la suerte de salir desordenado. Y acc_to_idx asigna indices
deliberadamente NO en orden de accesion, para que un test que pase no pueda estar
pasando por coincidencia.

El helper aparte: que ordena cualquier iterable (lista, reversed, set), que
descarta una accesion que el pool unificado no lleva en vez de petar, y que un
aspecto vacio da un array int32 vacio.

Un test existente, TestReferencePoolQuery::test_builds_subquery, afirmaba la
cadena de joins exacta y se actualiza, porque la consulta termina ahora en
.order_by().

Operativa, que no va en esta PR

Las caches de banco que ya hay en disco se escribieron con el orden viejo. Al
desplegar hay que invalidarlas para que se reconstruyan ordenadas, o el arreglo no
cambia nada en esta maquina hasta que caduquen solas. Lo hago en el despliegue.

El 91% de las consultas tienen vecinos a distancia ~0, ellas mismas o
gemelas con la misma secuencia, asi que los terminos candidatos empatan en
el k-esimo hueco y la busqueda admite al donante que vaya PRIMERO en el
banco. Dos sitios decidian ese orden por accidente.

1. LA CONSULTA DEL POOL UNIFICADO no llevaba ORDER BY, asi que el orden era
el que emitiera el plan. Medido sobre el banco de la 219, 142.422 filas,
las mismas filas siempre:

    hash join, sin paralelismo         89b79ad15383
    hash join, work_mem bajo           89b79ad15383
    merge join (enable_hashjoin=off)   cf77c750bed5
    las mismas filas ordenadas por accesion  cf77c750bed5

El plan por si solo movia f_micro_w ~1e-3 por celda, concentrado en CCO y
con signos mezclados, y el plan depende de estadisticas y de work_mem.

2. EL ARRAY DE INDICES POR ASPECTO se construia iterando un set de cadenas.
CPython aleatoriza el hash de cadenas por proceso salvo que PYTHONHASHSEED
este fijado, y aqui no lo esta: comprobado pasando las mismas ocho
accesiones por tres interpretes y obteniendo tres ordenes. Eso es peor que
lo anterior, porque cambiaba en CADA ARRANQUE de worker que reconstruyera
la cache, no solo cuando cambia el plan.

LA CACHE EN DISCO LOS TAPABA LOS DOS. Una repeticion identica reproducia
par a par, 187.834 pares con 0 solo en A, 0 solo en B y 0 con distancia
distinta, mientras una corrida posterior a la caducidad de la cache no lo
habria hecho. El enunciado real no era "dos corridas pueden discrepar",
era "una corrida de hoy puede discrepar de una de la semana que viene, en
silencio, por 1e-3 por celda".

Se ordena por accesion porque es el orden que una reproduccion externa
elegiria sin que nadie se lo diga, y porque alinea el desempate con el del
nodo de computo, cuyo banco ya va por accesion. Eso convierte el
desacuerdo medido entre los dos arneses, 4 celdas de 9 identicas y el
resto dentro de 8e-4, en coincidencia exacta.
@github-actions
github-actions Bot enabled auto-merge (squash) October 12, 2026 10:48
@codecov

codecov Bot commented Oct 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.02%. Comparing base (d8bc801) to head (8f961bd).
⚠️ Report is 106 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1007      +/-   ##
===========================================
+ Coverage    87.85%   88.02%   +0.17%     
===========================================
  Files          306      330      +24     
  Lines        23414    25583    +2169     
===========================================
+ Hits         20570    22520    +1950     
- Misses        2844     3063     +219     
Files with missing lines Coverage Δ
...operations/predict_go_terms/_batch_op_reference.py 99.47% <100.00%> (+<0.01%) ⬆️

... and 39 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions
github-actions Bot merged commit 7061b9c into develop Oct 12, 2026
21 checks passed
github-actions Bot pushed a commit that referenced this pull request Oct 12, 2026
…1007 (#1008)

#1007 se fusiono **sin estas correcciones**, que es lo que las hace
necesarias y
no una segunda pasada de gusto.

## Por que se quedaron fuera

| | |
|---|---|
| 10:46:46Z | empujo el primer commit de #1007 |
| 10:48:35Z | un bot del repo arma el auto-merge en modo SQUASH |
| **10:54:02Z** | **#1007 se fusiona**, con el primer commit solo |
| 11:20:08Z | escribo estas correcciones, 26 minutos despues del merge |

El `git push` del segundo commit funciono, pero a una rama cuya PR ya
estaba
cerrada, asi que el squash se quedo con 3 ficheros y 209 inserciones en
vez de 4
y 272. Lo que hay hoy en `develop` es la version que **no hace nada**.

## Lo que falta en develop, y lo primero lo deja en no-op

**1. Era un no-op.** El cambio altera QUE contiene un pool cacheado, su
orden, sin
tocar su identidad de cache. `_CACHE_EPOCH` sigue en 3 y `disk_cache.py`
no entro.
Los tres guardas son ciegos al orden: la carga de disco valida solo
`len(accessions) == expected_count` y el conjunto de filas no cambio; la
rama de
frescura se salta incluso esa cuenta; y los indices por aspecto los
guarda
`pool_fingerprint` calculado sobre las accesiones **recien cargadas de
esa misma
cache**, que es autorreferencial, asi que `_find_missing_aspects`
devuelve `[]` y
`_aspect_indices` no corre nunca.

Medido sobre la cache viva de esta maquina, clave
`e3__e7ba37da...__a4517ed0...__donor-f7658ad59248`: 142.422 accesiones
**sin
ordenar**, y los tres `.pool` registran `901a354f173edfa9`, que es la
huella de ese
orden viejo, donde el ordenado da `dc280304d5dce097`.

Epoca a 4. Es lo que su propio docstring dice que es el instrumento para
esto, y ya
se uso asi en agosto por un caso de la misma familia.

**2. El orden no era el de Python.** `ORDER BY` usaba la colacion de la
base,
`en_US.utf8`, cuya glibc ignora la puntuacion en la primera pasada: pone
`A0A18`
antes de `A0A1-9` y `P0DTC21` antes de `P0DTC2-3`. `_aspect_indices`
fija su mitad
con `sorted()` de Python, que es codepoint. Las dos mitades del arreglo
no
coincidian **entre si**. `COLLATE "C"`.

**3. No era un orden total.** `sequence_embedding` es una fila por
(sequence, config, chunk_start) y el join es por (sequence_id,
embedding_config_id), asi que con `use_chunking` cada accesion sale una
vez por
trozo y esas filas empatan bajo una sola clave. Y los trozos casi
duplicados de una
misma proteina son las entradas **mas** propensas a empatar, que es el
caso que este
orden existe para fijar. `chunk_index_s` de segunda clave. Ninguna
config de la
campana trocea hoy, asi que no cuesta nada ahora y quita una trampa
despues.

**4. Habia un tercer sitio, no dos.** `_collect_aspect_annotations`
tampoco ordena,
y las listas de anotaciones por donante se aplanan POSICIONALMENTE en el
CSR de
disco: el candidato toma su `evidence_code` de la primera fila que
encaja, y el
codigo multiplica la puntuacion. Ordenadas en Python y no con un `ORDER
BY` porque
esa consulta recorre 11.160.704 filas en la 219, y un sort de ese tamano
en este
Postgres es la patologia del derrame de `work_mem`; estas listas tienen
decenas de
entradas, asi que ordenarlas es lineal en filas ya en memoria.

## Los tests tambien estaban mal

**El aserto no distinguia el caso que importaba.** Tres de los cuatro
asertos del
`ORDER BY` pasaban con el orden metido DENTRO de la subconsulta de
anotadas, porque
`split("order by")[1]` toma todo lo que hay tras la PRIMERA aparicion, y
ahi detras
viene `) as anon_1 on protein.accession = ...`, que contiene la
subcadena.
Sustituido por un helper que exige una sola aparicion, mira la ULTIMA y
comprueba
que no queda `join` detras.

**Un docstring afirmaba lo contrario de lo que hace el test.** Decia que
`test_it_does_not_keep_the_order_it_was_handed` "pasaba trivialmente"
bajo el
defecto, cuando falla de forma determinista, y lo decia justo encima del
aserto.

**Probaba el helper pero no su llamada.** Borrar
`_pin_annotation_order(aspect_to_go_map)` dejaba los 19 tests en verde.
Anadido
`TestTheCollectorActuallyPinsTheOrder`, que es el mismo hueco que dejo
pasar los
defectos reales: un helper que funciona y al que nadie llega.

**Las siete mutaciones estan cazadas**, comprobadas una a una
revirtiendo cada
pieza: sin ORDER BY, ORDER BY en la subconsulta, sin COLLATE, sin
`chunk_index_s`,
indices sin ordenar, listas sin ordenar y epoca sin subir.

Y dos tests mios de la epoca eran duplicados exactos de
`test_the_cache_key_carries_an_epoch.py`. Quitados; el unico que faltaba
vive ahora
en ese fichero, con el segundo incidente documentado al lado del de
2026-08-19.

## De donde salio todo esto

De una revision adversarial de #1007 con cuatro lentes independientes y
verificacion por refutacion. Siete hallazgos sobrevivieron; cuatro eran
defectos
del propio cambio. Queda uno fuera de esta PR a proposito: un cuarto
banco de
donantes en `_protst_text.py` con el mismo `ORDER BY` ausente y un
`argpartition`
que rompe empates por posicion. Esa ruta esta apagada por defecto
(`compute_protst` en false), asi que va aparte.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant