Add Database Client by JBorrow · Pull Request #20 · simonsobs/socat

JBorrow · 2026-04-24T20:00:52Z

Adds a client interface that connects directly to the database.

Also refactors the core 'sub-services' to be properties of the client itself, so they are accessible in all cases.

Depends on #19.

The main change here is in db.py where I've added a new interface that's similar to mock but instead uses the more 'usable' database format. We will want to think very soon about how we ingest ephemeris here because it is currently very slow doing this on a row-by-row basis.

This required taking some of the 'complex' databsae functionality and creating statements.py that lists shared statements that are beyond minor crud operations between the asynchonrous and synchronous interfaces. I am not yet fully convinced that this is the right final approach that we should keep but the alternative (re-using the asynchronous layer inside a synchronous wrapper) is also not idea.

In addition I also centralized a bunch of database setup operations including making a mirror interface of the async system with its synchronous counterpart.

The new db.py comes along with tests for this new functionality.

This PR used GPT-5.3-Codex through GitHub Copilot for some initial work. I will use the Copilot system to review the code now that it is complete.

Copilot

Pull request overview

This PR introduces a synchronous, DB-backed SOCat client (parallel to the existing mock client) and refactors “sub-services” (astroquery/SSO/ephem) to be accessible as properties on a composite client. It also centralizes shared SQLModel/SQLAlchemy statements and session factory setup, and updates ingestion and tests to use the new client shape.

Changes:

Add socat/client/db.py DB-backed composite client with .astroquery, .sso, .ephem sub-clients.
Introduce socat/database/statements.py and refactor core async update/query helpers to use shared SQL statements.
Centralize sync/async session factories and schema initialization in socat/database/session.py, updating API imports and adding DB client tests.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
tests/test_sso_mock.py	Refactors mock tests to use `mock_client.sso` / `mock_client.ephem`.
tests/test_sso_api.py	Adjusts API test expectations for cascade deletion behavior.
tests/test_mock.py	Updates mock tests to expect `astroquery` “no results” warnings and uses `.astroquery` sub-client.
tests/test_db_client.py	Adds coverage for DB-backed client CRUD, cascade behavior, and not-found semantics.
tests/conftest.py	Adds `db_client` fixture and refactors mock sub-client fixtures to derive from `mock_client`.
socat/ingest/jplparquet.py	Switches ingestion API to accept a composite client and changes mock-db builder implementation.
socat/database/statements.py	Adds shared SQL statement builders (create_name, box query, update helpers).
socat/database/session.py	New module for shared sync/async session factories and schema initialization.
socat/core/services.py	Refactors async update to use shared `statements.update_service`.
socat/core/moving_sources.py	Refactors async update to use shared `statements.update_ephem`.
socat/core/fixed_sources.py	Refactors box query and update to use shared statements.
socat/client/settings.py	Adds `"db"` backend option to client settings.
socat/client/mock.py	Adds `.astroquery`, `.sso`, `.ephem` properties to the mock composite client.
socat/client/db.py	New DB-backed composite client and sub-clients.
socat/client/core.py	Extends `ClientBase` with abstract `.astroquery` / `.sso` / `.ephem` properties.
socat/api/routers/*.py	Updates router imports to use centralized `SessionDependency`.
socat/api/async_ses.py	Removes duplicated async session dependency module.
socat/api/app.py	Uses centralized async schema initialization during FastAPI lifespan.
socat/api/init.py	Re-exports `SessionDependency` from the new centralized module.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sulla2012 · 2026-04-27T19:06:25Z

+    ) -> RegisteredFixedSource:
+        position, name, flux = statements.create_name(name, astroquery_service)
+


I think this is supposed to raise a runtime error HTTPException if the returned source is None unless that error is caught higher up.

Ah ok this seems to be handled in routers.fixed_sources in a change that for some reason is not in this PR as I see it.

Ok sorry I confused myself reviewing this, this is for the mock database, so routers.fixed_sources is irrelevant. In that case I disagree with copilot, the database.statements.create_name function handles a no-result by raising a ValueError, not returning none. We can either accept the error handling as implemented in database.statements.create_name or use a try-except block here if we need to modify this instance of the error handling i.e. if we want this function to return None instead of raising the ValueError if there is a no-return of the querry.

Sulla2012 · 2026-04-27T21:16:39Z

+def create_name(
+    name: str, astroquery_service: str
+) -> tuple[ICRS, str, Quantity | None]:
+    """"""
+    service: BaseVOQuery = getattr(
+        import_module(f"astroquery.{astroquery_service.lower()}"),
+        astroquery_service,
+    )
+
+    requested_params = ["ra", "dec"]
+
+    result_table = service.query_object(name)
+    result_table["ra"].convert_unit_to("deg")
+    result_table["dec"].convert_unit_to("deg")
+    if "flux" in result_table.columns:
+        result_table["flux"].convert_unit_to("mJy")  # pragma: no cover
+    result_dict = {param: None for param in requested_params}
+    if len(result_table) == 0:
+        return None
+    for param in requested_params:


Copilot is confused here but there does seem to be a type annotation error since create_name only raises an error the type annotation should be tuple[ICRS, str, Quantity].

Sulla2012 · 2026-04-27T19:06:25Z

+    ) -> RegisteredFixedSource:
+        position, name, flux = statements.create_name(name, astroquery_service)
+


I think this is supposed to raise a runtime error HTTPException if the returned source is None unless that error is caught higher up.

Sulla2012 · 2026-04-27T19:10:18Z

+    ) -> RegisteredFixedSource:
+        position, name, flux = statements.create_name(name, astroquery_service)
+


Ah ok this seems to be handled in routers.fixed_sources in a change that for some reason is not in this PR as I see it.

Sulla2012 · 2026-04-27T19:28:55Z

+    ) -> RegisteredFixedSource:
+        position, name, flux = statements.create_name(name, astroquery_service)
+


Ok sorry I confused myself reviewing this, this is for the mock database, so routers.fixed_sources is irrelevant. In that case I disagree with copilot, the database.statements.create_name function handles a no-result by raising a ValueError, not returning none. We can either accept the error handling as implemented in database.statements.create_name or use a try-except block here if we need to modify this instance of the error handling i.e. if we want this function to return None instead of raising the ValueError if there is a no-return of the querry.

Sulla2012 · 2026-04-27T19:32:52Z

+                statements.get_box(lower_left=lower_left, upper_right=upper_right)
+            )
+
+            return [s.to_model() for s in sources.scalars().all()]


Compared to the above the no-result case seems not to be handled here or in database.statements. That seems to have been the behavior previous (which I wrote) so that's fine. We do seem to have lost the error handling on the box bounds unless I'm missing something.

Sulla2012 · 2026-04-27T19:35:07Z

+    def get_source(self, *, source_id: int) -> RegisteredFixedSource | None:
+        with self._get_session() as session:
+            source = session.get(RegisteredFixedSourceTable, source_id)
+            if source is None:


Small difference in functionality from core.fixed_sources.get_source which raises a ValueError instead of returning None.

Sulla2012 · 2026-04-27T19:37:08Z

+        with self._get_session() as session:
+            source = session.get(SolarSystemObjectTable, sso_id)
+
+            if source is None:


None vs ValueError

Sulla2012 · 2026-04-27T19:37:17Z

+            )
+
+            source_list = [s.to_model() for s in sources.scalars().all()]
+            if len(source_list) == 0:


None vs ValueError

Sulla2012 · 2026-04-27T19:37:30Z

+            )
+
+            source_list = [s.to_model() for s in sources.scalars().all()]
+            if len(source_list) == 0:


None vs ValueError.

Sulla2012 · 2026-04-27T21:13:04Z

+        with self._get_session() as session:
+            ephem = session.get(RegisteredMovingSourceTable, ephem_id)
+
+            if ephem is None:


None vs ValueError

Sulla2012 · 2026-04-27T21:16:39Z

+def create_name(
+    name: str, astroquery_service: str
+) -> tuple[ICRS, str, Quantity | None]:
+    """"""
+    service: BaseVOQuery = getattr(
+        import_module(f"astroquery.{astroquery_service.lower()}"),
+        astroquery_service,
+    )
+
+    requested_params = ["ra", "dec"]
+
+    result_table = service.query_object(name)
+    result_table["ra"].convert_unit_to("deg")
+    result_table["dec"].convert_unit_to("deg")
+    if "flux" in result_table.columns:
+        result_table["flux"].convert_unit_to("mJy")  # pragma: no cover
+    result_dict = {param: None for param in requested_params}
+    if len(result_table) == 0:
+        return None
+    for param in requested_params:


Copilot is confused here but there does seem to be a type annotation error since create_name only raises an error the type annotation should be tuple[ICRS, str, Quantity].

JBorrow marked this pull request as draft April 24, 2026 20:01

JBorrow force-pushed the add-database-client branch 2 times, most recently from 947ec44 to 8f0879c Compare April 27, 2026 14:38

JBorrow added 8 commits April 27, 2026 10:39

Added base for database client

0e1b972

Make 'sub-catalogs' properties on main catalog

1dfca88

Use pre-defined client.

0c190b5

Create database tables on startup

4a88200

Remove deletion for ephems that should have been removed by cascade

b2a944c

Begun shared select statements between two service layers

48208ed

Applied new pattern to astroquery

711554b

Migrate final shared statements

a22ee6f

JBorrow force-pushed the add-database-client branch from 8f0879c to a22ee6f Compare April 27, 2026 14:39

JBorrow requested a review from Copilot April 27, 2026 14:42

Copilot started reviewing on behalf of JBorrow April 27, 2026 14:43 View session

Copilot AI reviewed Apr 27, 2026

View reviewed changes

Resovle copilot comments

6807e7d

JBorrow marked this pull request as ready for review April 27, 2026 15:12

JBorrow requested a review from Sulla2012 April 27, 2026 15:13

Sulla2012 reviewed Apr 27, 2026

View reviewed changes

		) -> RegisteredFixedSource:
		position, name, flux = statements.create_name(name, astroquery_service)

Conversation

JBorrow commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JBorrow commented Apr 24, 2026 •

edited

Loading