Skip to content

DM-53501: Investigate Null values in camera_hexapod_compensation_offset_* column #122

Merged
rcboufleur merged 14 commits into
mainfrom
tickets/DM-53501
Jul 20, 2026
Merged

DM-53501: Investigate Null values in camera_hexapod_compensation_offset_* column #122
rcboufleur merged 14 commits into
mainfrom
tickets/DM-53501

Conversation

@rcboufleur

Copy link
Copy Markdown
Contributor

Nulls in the camera_hexapod_compensation_offset_* columns came from two places. First, the upsert was overwriting good data with nulls whenever a topic returned no results for a given exposure, the incoming row had a null in that column, and the old ON CONFLICT ... SET blindly replaced the stored value. Second, exposures that straddled the task boundary were being processed with an incomplete time window, producing partial computations. The upsert now uses COALESCE(excluded.column, table.column) to keep the existing value when the incoming one is null, and the processing loop only includes exposures and visits whose timespan falls entirely inside the task window.

A few supporting cleanups are included: DAO objects are cached per Transform instance instead of reconstructed on every run, the database pool uses QueuePool with pool_pre_ping instead of NullPool, and SAWarning output is routed to the structured logger. Latent bugs fixed along the way: task_update_counts was writing "failed" instead of "running", select_failed's butler_repo filter was dead code, and current_utc could raise NameError depending on which branch executed.

@rcboufleur
rcboufleur requested review from bbrondel and Copilot and removed request for Copilot June 23, 2026 14:33

@bbrondel bbrondel left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have concerns about exposures getting discarded if they straddle an EFD query window. Please write up a Jira ticket to make sure that will get addressed and write your ticket number into an appropriate place in a code comment. Other than that, approved with the following comments.

self.dialect = postgresql
else:
raise Exception(f"The dialect for {sgbd} has not yet been implemented.")
raise NotImplementedError(f"The dialect for {sgbd} has not yet been implemented.")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move lines 92-99 above the initialization of self._engines to get the intended exception message you had in mind.

for uri in self.db_uris
]

sgbd = self.db_uri.split(":")[0]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should probably be a check that all URIs in the list have the same dialect.

pool_size=1,
max_overflow=0,
pool_pre_ping=True,
connect_args={"connect_timeout": 5},

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The connect_timeout arg doesn't work with sqlite. Do something like this instead:

if sgbd == "sqlite":
    connect_args = {"timeout": 5 }
    self.dialect = sqlite
elif sgbd == "postgresql":
    connect_args = {"connect_timeout": 5 }
    self.dialect = postgresql
else:
    raise ...

@rcboufleur

Copy link
Copy Markdown
Contributor Author

@bbrondel
About the concerns of discarting exposures/visits, the timewindow is meant to cover this case. In process_task(), exposures/visits are queried with an expanded window (start - timewindow, end + timewindow), with timewindow currently set to 1 minute. Then _process_interval() discards only records that are not fully contained in that expanded window. Since exposures are 15s and visits are 30s, anything straddling the original task boundary should still fit inside the current buffer. Records crossing the expanded boundary are skipped to avoid computing values from incomplete EFD data. We can increase timewindow if needed, but we keep it as small as possible to avoid unnecessary load on InfluxDB.

@rcboufleur
rcboufleur merged commit a3a0b7e into main Jul 20, 2026
12 checks passed
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.

2 participants