Skip to content

Commit b23102f

Browse files
feat!: UTA access class separation of concerns, postgres lib fixes (#465)
Basically a lot of tech debt fixes * Migrate from `asyncpg` to `psycopg`. Honestly, asyncpg might be faster but we are generally using psycopg elsewhere so might as well stick with one thing. * **(breaking)** break the DB access class into 2 things: 1) a repository pattern-style lookup class that just lasts as long as a DB connection, and 2) a connection pool-holder class for easily launching repository instances. * This means that applications which want to have more direct control over the lifespan of the connection pool can do so directly, and separates the connection-management logistics from the actual query/data transformation logic. * Mark the "get AWS secrets" function as deprecated. In the back of my head, I felt like we were trying to move away from secrets manager for DB connection params, but maybe I made that up. Either way, it's not ideal to be writing code specific to our AWS deployment into a library, so I threw down a deprecation warning and maybe we can figure out something better in 10 years * Previously we were basically doing a connection pool status check every time we ran a query, and (re)creating the pool if it was gone. It's a little funky to be delaying pool setup/config until runtime like this. I included a child class of the new `UtaDatabase` class that basically retains this behavior so we can minimize needed changes in the short term (+ less work for me to update tests) * **(breaking in the future)** set schema in the way you are supposed to per postgres docs: ie `postgresql://uta_admin@localhost:5432/uta?options=-csearch_path%3Duta_20241220,public` rather than `/uta/uta_20241220` * the `postgresql://stuff/(db_name)/(schema_name)` thing is nonstandard (I think it was basically invented for the hgvs library) and not in keeping with more typical postgres access patterns. In general, there are two standards for describing postgres connections -- as a [key-value string](https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNSTRING-KEYWORD-VALUE) and as a [URI](https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNSTRING-KEYWORD-VALUE). So, I think if we're gonna have users go with the URI way, we should adhere to the standard. * env var name stays the same, but the old value won't work any more (it'll give you the wrong search path) and the default value is new * **update**: still support the old URL format via a `_normalize_uta_db_url` function that converts it to the new way and issues a deprecation warning. If this ends up being really annoying we can also just silence the warning. * Don't hardcode schema into queries anymore. Instead, set the search path when creating the connection. (Still assume `uta_20241220` by default, but this is set at connection pool creation via the connection string) * Don't hard-code variables into queries anymore, but pass them as params using the psycopg `%(argname)s` syntax * Except for 1-2 exceptions that were too hard, make all queries static instead of dynamically constructing them * Allow the public UTA query execution function to accept params as an additional arg * Change one UTA DB function that returned a `(result, failure_description)` tuple where the result and the failure values were mutually exclusive into a function that returned the result value, and raised an exception in case of failure. * Remove all references to the `genomic` table (I think this addresses #430) * Move all uses of the UTA DB `execute_query` method in other modules into dedicated UTA db queries. We offer this method publicly just in case some external user wants to run a manual query, but in general, I think it's better to move all coolseqtool UTA queries into one module. basically I think the ideal FastAPI usage for just UTA should look like ```python @asynccontextmanager async def lifespan(app: FastAPI) -> AsyncGenerator: uta_pool = await create_uta_connection_pool() app.state.uta_pool = uta_pool yield await uta_pool.close() async def get_uta(request: Request) -> Generator(UtaRepository, None, None): async with request.app.state.uta_pool.connection() as conn: yield UtaRepository(conn) @app.get("/check_exists") async def check_exists(gene: str, uta: Annotated[UtaRepository, Depends(get_uta)]): return await uta.gene_exists(gene) ``` and within coolseqtool itself, or in other contexts where you might want to pass around the entire coolseqtool god class instance, you can do something like ```python uta_db: UtaDatabase # assume this exists async with uta_db.repository() as uta: print(await uta.gene_exists("BRAF")) ``` A few misc engineering notes I thought about while working on this * Yes, we want to be using connection pools vs plain connections in basically all contexts. The pool more efficiently recycles connections, so even in contexts where everything is sequential, there's still a speedup * We probably want to stay async. It would be a pain to move back. Maybe in the future we can move the rest of the library to async/dial down all the blocking file open calls. --------- Co-authored-by: Kori Kuzma <korikuzma@gmail.com>
1 parent 15bd9f3 commit b23102f

15 files changed

Lines changed: 1129 additions & 1032 deletions

File tree

compose.yaml

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ services:
1414
read_only: true
1515
bind:
1616
create_host_path: false
17-
- ./uta-setup.sql:/docker-entrypoint-initdb.d/uta-setup.sql
1817
ports:
1918
- 127.0.0.1:5432:5432
2019

docs/source/usage.rst

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ Individual classes will accept arguments upon initialization to set parameters r
8080
* - ``SEQREPO_ROOT_DIR``
8181
- Path to SeqRepo directory (i.e. contains ``aliases.sqlite3`` database file, and ``sequences`` directory). Used by :py:class:`SeqRepoAccess <cool_seq_tool.handlers.seqrepo_access.SeqRepoAccess>`. If not defined, defaults to ``/usr/local/share/seqrepo/latest``.
8282
* - ``UTA_DB_URL``
83-
- A `libpq connection string <https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNSTRING>`_, i.e. of the form ``postgresql://<user>:<password>@<host>:<port>/<database>/<schema>``, used by the :py:class:`UtaDatabase <cool_seq_tool.sources.uta_database.UtaDatabase>` class. By default, it is set to ``postgresql://anonymous@localhost:5432/uta/uta_20241220``.
83+
- A `libpq connection URI <https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNSTRING-URIS>`_, i.e. of the form ``postgresql://<user>:<password>@<host>:<port>/<database>?options=search_path%3D<schema>,public``, used by the :py:class:`UtaDatabase <cool_seq_tool.sources.uta_database.UtaDatabase>` class. By default, it is set to ``postgresql://anonymous@localhost:5432/uta?options=-csearch_path%3Duta_20241220,public``.
8484
* - ``LIFTOVER_CHAIN_37_TO_38``
8585
- A path to a `chainfile <https://genome.ucsc.edu/goldenPath/help/chain.html>`_ for lifting from GRCh37 to GRCh38. Used by the :py:class:`LiftOver <cool_seq_tool.mappers.liftover.LiftOver>` class as input to `agct <https://pypi.org/project/agct/>`_. If not provided, agct will fetch it automatically from UCSC.
8686
* - ``LIFTOVER_CHAIN_38_TO_37``

pyproject.toml

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ description = "Common Operation on Lots of Sequences Tool"
2424
license = "MIT"
2525
license-files = ["LICENSE"]
2626
dependencies = [
27-
"asyncpg",
27+
"psycopg[pool]",
2828
"boto3",
2929
"agct >= 0.2.0rc1",
3030
"polars ~= 1.0",
@@ -37,6 +37,9 @@ dependencies = [
3737
dynamic = ["version"]
3838

3939
[project.optional-dependencies]
40+
pg_binary = [
41+
"psycopg[binary, pool]",
42+
]
4043
dev = [
4144
"prek>=0.2.23",
4245
"ipython",
@@ -46,7 +49,7 @@ dev = [
4649
]
4750
tests = ["pytest", "pytest-cov", "pytest-asyncio", "mock"]
4851
docs = [
49-
"sphinx==6.1.3",
52+
"sphinx==6.2.1",
5053
"sphinx-autodoc-typehints==1.22.0",
5154
"sphinx-autobuild==2021.3.14",
5255
"sphinx-copybutton==0.5.2",

src/cool_seq_tool/app.py

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,10 @@
22
data handler and mapping resources for straightforward access.
33
"""
44

5-
import logging
65
from pathlib import Path
76

87
from biocommons.seqrepo import SeqRepo
8+
from psycopg_pool import AsyncConnectionPool
99

1010
from cool_seq_tool.handlers.seqrepo_access import SEQREPO_ROOT_DIR, SeqRepoAccess
1111
from cool_seq_tool.mappers import (
@@ -16,9 +16,7 @@
1616
)
1717
from cool_seq_tool.sources.mane_transcript_mappings import ManeTranscriptMappings
1818
from cool_seq_tool.sources.transcript_mappings import TranscriptMappings
19-
from cool_seq_tool.sources.uta_database import UTA_DB_URL, UtaDatabase
20-
21-
_logger = logging.getLogger(__name__)
19+
from cool_seq_tool.sources.uta_database import LazyUtaDatabase, UtaDatabase
2220

2321

2422
class CoolSeqTool:
@@ -40,7 +38,7 @@ def __init__(
4038
transcript_file_path: Path | None = None,
4139
lrg_refseqgene_path: Path | None = None,
4240
mane_data_path: Path | None = None,
43-
db_url: str = UTA_DB_URL,
41+
uta_connection_pool: AsyncConnectionPool | None = None,
4442
sr: SeqRepo | None = None,
4543
force_local_files: bool = False,
4644
) -> None:
@@ -75,8 +73,10 @@ def __init__(
7573
:param transcript_file_path: The path to ``transcript_mapping.tsv``
7674
:param lrg_refseqgene_path: The path to the LRG_RefSeqGene file
7775
:param mane_data_path: Path to RefSeq MANE summary data
78-
:param db_url: PostgreSQL connection URL
79-
Format: ``driver://user:password@host/database/schema``
76+
:param uta_connection_pool: pyscopg connection pool to UTA instance. If not
77+
provided, a lazy UTA connection will be used, meaning the connection won't
78+
be initiated until the first attempted UTA query, and will use environment
79+
configs/library defaults
8080
:param sr: SeqRepo instance. If this is not provided, will create a new instance
8181
:param force_local_files: if ``True``, don't check for or try to acquire latest
8282
versions of static data files -- just use most recently available, if any
@@ -92,7 +92,10 @@ def __init__(
9292
self.mane_transcript_mappings = ManeTranscriptMappings(
9393
mane_data_path=mane_data_path, from_local=force_local_files
9494
)
95-
self.uta_db = UtaDatabase(db_url=db_url)
95+
if uta_connection_pool:
96+
self.uta_db = UtaDatabase(uta_connection_pool)
97+
else:
98+
self.uta_db = LazyUtaDatabase()
9699
self.alignment_mapper = AlignmentMapper(
97100
self.seqrepo_access, self.transcript_mappings, self.uta_db
98101
)

src/cool_seq_tool/mappers/alignment.py

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,8 @@ async def p_to_c(
4848
* Warning, if unable to translate to cDNA representation. Else ``None``
4949
"""
5050
# Get cDNA accession
51-
temp_c_ac = await self.uta_db.p_to_c_ac(p_ac)
51+
async with self.uta_db.repository() as uta:
52+
temp_c_ac = await uta.p_to_c_ac(p_ac)
5253
if temp_c_ac:
5354
c_ac = temp_c_ac[-1]
5455
else:
@@ -89,7 +90,8 @@ async def _get_cds_start(self, c_ac: str) -> tuple[int | None, str | None]:
8990
- CDS start site if found. Else ``None``
9091
- Warning, if unable to get CDS start. Else ``None``
9192
"""
92-
cds_start_end = await self.uta_db.get_cds_start_end(c_ac)
93+
async with self.uta_db.repository() as uta:
94+
cds_start_end = await uta.get_cds_start_end(c_ac)
9395
if not cds_start_end:
9496
cds_start = None
9597
warning = f"Accession {c_ac} not found in UTA db"
@@ -149,12 +151,13 @@ async def c_to_g(
149151
c_start_pos -= 1
150152

151153
# Get aligned genomic and transcript data
152-
genomic_tx_data = await self.uta_db.get_genomic_tx_data(
153-
c_ac,
154-
(c_start_pos + cds_start, c_end_pos + cds_start),
155-
AnnotationLayer.CDNA,
156-
target_genome_assembly=target_genome_assembly,
157-
)
154+
async with self.uta_db.repository() as uta:
155+
genomic_tx_data = await uta.get_genomic_tx_data(
156+
c_ac,
157+
(c_start_pos + cds_start, c_end_pos + cds_start),
158+
AnnotationLayer.CDNA,
159+
target_genome_assembly=target_genome_assembly,
160+
)
158161

159162
if not genomic_tx_data:
160163
warning = (

0 commit comments

Comments
 (0)