Closes #145 by lcalisto · Pull Request #209 · NaturalGIS/seis-lab-data

lcalisto · 2026-09-10T11:35:20Z

Closes #145

First version of automatic preview generation, hooked into discovery

New tasks/derivers/ package (mirrors tasks/extractors/) renders a raster into a small WEBP (warped to 4326, percentile stretch, nodata as transparency), stored on the record as a derived asset (asset_type={THUMBNAIL,PREVIEW}, bytes in data, bounds in geog, no relative_path) via the DerivedRecordAssetCreate schema from Closes #175 #192
Discovery enqueues generation on a dedicated previews dramatiq queue (new compose worker; the existing one is now pinned to default). Enqueue is missing-only, the task is replace-all under a per-record advisory lock, so generation is idempotent and re-running discovery is also the recovery path
Eligibility = prefix match against config/preview-folders.json ( real folder list from IPMA) .

This v1 covers rasters that declare their own CRS (.tif/.tiff). Still to be done next: viewer that actually shows these previews (#154), then more formats as separate issues: XYZ via the mission's implicit_crs, vector via Martin ??, KMALL, and SEG-Y (just an icon, still to be decided).

Compose stacks need to gain a previews-worker service.

Note: the issue text says derived datasets are not to be generated automatically. In the July meeting we decided generation is automatic, hooked into discovery so users dont need to manage it. The task is trigger agnostic (idempotent), so a manual button can still be added later if a use case appears.

ricardogsilva

This is a very commendable effort.

Unfortunately, it seems based on a different assumption than the one that drove the original design - this is maybe my fault for not having made it clearer. I apologize for that 🥲

Letting the generation of derived assets run together with the discovery was meant to mean that the discovery runs first and then it automatically triggers an event to kickstart the derivation - it was not meant to mean that derivation would run intertwined with the discovery. Having discovery and derivation being two independent processes means that we can call any of them in isolation, if needed.

I'm requesting a change, but I'd also be open to changing my mind, if you can convince me that this is a better way than the original plan 😉 - let me know your thoughts

ricardogsilva · 2026-09-29T17:46:25Z

I'd have this configuration moved to the asset discovery configuration model. Not necessarily on this PR, but we will need to do it at some point.

Also a very minor nitpick - I would prefer the term 'directory' instead of 'folder', in order to keep it homogeneous with the rest of the code. The term folder is more of a GUI-related metaphor, and seems less appropriate here - I don't mean that this needs to be changed for this PR to be merged, it is just a minor note

Renamed to directory. Moving it into the asset discovery configuration model. Fully agree, issue #213

ricardogsilva · 2026-09-29T17:49:53Z

There is no bind volume for the config directory here - does this mean we have nothing in CI that needs it? Would it also mean that we don't need this additional previews-worker service?

If we don't need this service in CI, perhaps we can not define it, in order to conserve resources?

Right on both the worker is gone from the CI compose. The config bind is not needed there , the file goes inside the image.

ricardogsilva · 2026-09-29T17:52:53Z

There is no definition of the volume bind for config here either. In this case it seems this would be required for the production deployment to work appropriately.

Not needed. the Dockerfile copies the whole thing, prod reads the copied file. It can still be overridden per environment via SEIS_LAB_DATA__PREVIEW_DIRECTORIES_PATH.

ricardogsilva · 2026-09-29T17:54:10Z

+    # extra args are passed straight to dramatiq, which is how a worker gets
+    # pinned to a queue (`run-processing-worker --queues previews`)


This should be part of the docstring.

ricardogsilva · 2026-09-29T18:03:09Z

+    Two discovery runs may ask for previews of the same record at the same time,
+    and interleaved deletes and inserts would then commit duplicates, so the
+    replacement is serialized per record with an advisory lock and done in a
+    single transaction.
+    """
+    await session.execute(
+        text("SELECT pg_advisory_xact_lock(hashtext(:record_id))"),
+        {"record_id": str(survey_related_record.id)},
+    )


I'm not sure we need to reach for pg's explicit locking mechanism.

We already have the constants.SurveyRelatedRecordStatus.UNDER_DERIVATION status, which can be set on the record - This is supposed to prevent two simultaneous derivation operations on the same record.

I imagine that the derivation process would start by checking the record's status property and refuse to proceed unless it had a value of either DRAFT or PUBLISHED.

Maybe you looked into this and have a different opinion though?

Lock removed. The flip is a single conditional UPDATE, held only around the write of the derived assets, so a worker killed mid-render cannot strand a record in UNDER_DERIVATION.

ricardogsilva · 2026-09-29T18:11:01Z

+        # they become stale as soon as those change; the advisory lock keeps an
+        # in-flight preview task from re-inserting them after this delete
+        await session.execute(
+            text("SELECT pg_advisory_xact_lock(hashtext(:record_id))"),
+            {"record_id": str(survey_related_record.id)},
+        )


As mentioned above, I'd prefer to not resort to using pg explicit locks unless absolutely necessary.

I'm thinking that the operations layer, (which is where most coordination logic should be living) needs to check the current status of a record before calling the db command. If a record is not currently in either DRAFT or PUBLISHED state, then it would simply not be possible to have it be updated. This was the original plan anyway.

The original idea was to have asset derivation be an additional task, after discovery had already been done. We may need to rethink this now.

Removed as well ,the update operation now refuses records that are not DRAFT or PUBLISHED

ricardogsilva · 2026-09-29T18:15:51Z

+                # re-running discovery is the recovery path for records whose
+                # previews are missing, so only those get enqueued again
+                if deriver_dispatch.is_previewable(
+                    str(found_path), relative_file_path, preview_folders
+                ):
+                    existing_record_id = identifiers.SurveyRelatedRecordId(
+                        existing_asset.survey_related_record_id
+                    )
+                    has_derived_assets = any(
+                        constants.AssetType.DATA not in asset.asset_type
+                        for asset in await asset_queries.collect_all_record_assets(
+                            session, existing_record_id
+                        )
+                    )
+                    if not has_derived_assets:
+                        preview_tasks.generate_record_previews.send(
+                            raw_request_id=str(request_id),
+                            raw_survey_related_record_id=str(existing_record_id),
+                            raw_initiator=json.dumps(dataclasses.asdict(user)),
+                        )


The original plan was to have discovery decoupled from aux asset derivation, as these are two conceptually different operations. Even if we may perform derivation automatically, after discovery, like you mention in the PR description, the two should still be different and individually addressable operations.

I imagine that after discovery is finished there would be something like a DISCOVERY_FINISHED event which is sent and which would trigger a handler process to then start the derivation process.

Reworked as sugested. discovery's final step enqueues one mission-level derivation task, and the derivation operation is independently callable.

ricardogsilva · 2026-09-29T18:17:52Z

+    return frozenset(json.loads(contents)["folders"])
+
+
+async def generate_record_previews(


As mentioned before, this operation needs to check the record's current status in order to know whether to allow mutation of the record

Done! operation skips records that are not DRAFT or PUBLISHED

lcalisto · 2026-10-01T22:49:31Z

This is a very commendable effort.

Unfortunately, it seems based on a different assumption than the one that drove the original design - this is maybe my fault for not having made it clearer. I apologize for that 🥲

Letting the generation of derived assets run together with the discovery was meant to mean that the discovery runs first and then it automatically triggers an event to kickstart the derivation - it was not meant to mean that derivation would run intertwined with the discovery. Having discovery and derivation being two independent processes means that we can call any of them in isolation, if needed.

I'm requesting a change, but I'd also be open to changing my mind, if you can convince me that this is a better way than the original plan 😉 - let me know your thoughts

Thanks for the great review @ricardogsilva ! Agree with most of what you raised and no need to apologize, I'm really happy with what we are achieving!

Discovery and derivation are now two independent operations, the discovery operation's final step just enqueues one mission-level derivation task, and the derivation operation is callable on its own (which also leaves room for a manual "(re)generate" trigger later, if we ever want one). No event-handler infrastructure for now, let me know if you had something more elaborate in mind.
The pg advisory locks are gone. Coordination now uses UNDER_DERIVATION in the operations layer, with one important detail : the status flip is a single conditional update (UPDATE ... SET status = 'UNDER_DERIVATION' WHERE id = ... AND status = <status read at fetch>) held only around the write of the derived assets, not around the whole rendering, a worker killed mid-render (we use max_retries=0) can then never leave a record stranded in UNDER_DERIVATION, and update operations refuse records that are not DRAFT/PUBLISHED.
previews-worker removed from the CI compose (nothing consumes the queue there, the integration tests run on the stub broker) .
About the config binds: the file ships inside the image (the Dockerfile copies the whole tree), so CI and prod read the inside copy, only dev needs the bind.
Moving this configuration into the asset discovery configuration model: agreed, tracked in Move preview configuration into the asset discovery configuration model #213.

preview derivers package for rasters

43ceee8

lcalisto self-assigned this Sep 10, 2026

lcalisto and others added 3 commits September 10, 2026 20:26

preview folders config with real IPMA list

2019f17

preview generation task, operation and replace

48f11b1

enqueue preview generation and dedicated worker

b907df1

lcalisto requested a review from ricardogsilva September 12, 2026 16:29

lcalisto marked this pull request as ready for review September 12, 2026 18:55

lcalisto mentioned this pull request Sep 24, 2026

#154 preview viewer #211

Draft

ricardogsilva requested changes Sep 29, 2026

View reviewed changes

rework preview generation as review NaturalGIS#145

c2ef3fa

lcalisto mentioned this pull request Oct 1, 2026

Move preview configuration into the asset discovery configuration model #213

Open

lcalisto requested a review from ricardogsilva October 1, 2026 23:04

		# extra args are passed straight to dramatiq, which is how a worker gets
		# pinned to a queue (`run-processing-worker --queues previews`)

		return frozenset(json.loads(contents)["folders"])


		async def generate_record_previews(

Conversation

lcalisto commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ricardogsilva left a comment

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!

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!

lcalisto Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

lcalisto commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lcalisto commented Sep 10, 2026 •

edited

Loading

lcalisto Oct 1, 2026 •

edited

Loading

lcalisto commented Oct 1, 2026 •

edited

Loading