Skip to content

Conversation

ktlim
Copy link
Contributor

@ktlim ktlim commented Mar 8, 2023

No description provided.

@ktlim ktlim force-pushed the tickets/DM-38269 branch 19 times, most recently from 0f43b24 to 61c0398 Compare March 15, 2023 23:27
@ktlim ktlim marked this pull request as ready for review March 15, 2023 23:35
@ktlim ktlim force-pushed the tickets/DM-38269 branch 2 times, most recently from aaea135 to 244802f Compare March 17, 2023 01:07
@ktlim ktlim force-pushed the tickets/DM-38269 branch from dd29d2b to 53632d2 Compare April 6, 2023 00:06
@ktlim ktlim requested a review from kfindeisen April 6, 2023 00:17
Copy link
Member

@kfindeisen kfindeisen left a comment

Choose a reason for hiding this comment

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

I'm a bit concerned about the way this has been implemented -- the new Snap.is_consistent method breaks down a lot of the careful separation of functionality you yourself had insisted on in the beginning, and I don't understand why the new code looks for raws in the central repo. I think I'd at least like to understand the motivation for these changes a little better.

"""
datasets = set(self.central_butler.registry.queryDatasets(
datasetType="raw",
collections=f"{instrument}/raw/all",
Copy link
Member

Choose a reason for hiding this comment

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

This is an unsafe operation for two reasons:

  • As I understand it, raws are supposed to be ingested to the central repo independently of the PP workflow, and the prototype code will no longer do so on or after DM-36051. That makes this check unreliable for deciding if raws are available.
  • Because the repo is shared among all workers, self.central_butler is prone to concurrency problems. I'd recommend keeping the number of central_butler operations to the absolute minimum; if it really is necessary to query it here, then please call self.central_butler.registry.refresh() first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm no longer doing this — but I'm no longer doing anything. Since the Kafka consumer is subscribed at the beginning of next_visit_handler, I think it's only cold starts that could have an image show up before the subscription. Otherwise, all image notifications should be picked up by consumer.consume().

@ktlim ktlim force-pushed the tickets/DM-38269 branch 2 times, most recently from d20fdfb to 8b1c929 Compare May 3, 2023 04:25
@ktlim ktlim requested a review from hsinfang May 3, 2023 04:28
@ktlim
Copy link
Contributor Author

ktlim commented May 3, 2023

I've redone this with far fewer changes to activator.py but totally getting rid of the Snap class in raw.py and rewriting it so that it can handle both LSST (using JSON sidecar) and non-LSST cameras. Note that there is one thing that is still TODO for LSST cameras: looking for past images that might match this visit when about to start waiting for new images.

I have a bunch of LSST-specific code that recalculates certain things like detector numbers and exposure ids. I was reluctant to use obs_lsst for this, but I think that is false economy, as we already need it to be able to ingest. I'll work on incorporating that.

@ktlim ktlim force-pushed the tickets/DM-38269 branch 3 times, most recently from e4f159b to 94bd0e6 Compare May 3, 2023 21:41
@ktlim ktlim force-pushed the tickets/DM-38269 branch from 94bd0e6 to ff268d8 Compare May 4, 2023 00:18
ktlim added 2 commits May 3, 2023 17:29
This allows LSST cameras to use different path patterns from other
cameras, and it allows LSST cameras to get their group information from
a "sidecar" JSON file rather than the path.

Determining based on a group id whether the images associated with it
have already arrived works for non-LSST cameras but is left for a future
ticket for the LSST cameras.
@ktlim ktlim force-pushed the tickets/DM-38269 branch from ff268d8 to 2ce2696 Compare May 4, 2023 00:30
@ktlim ktlim merged commit d74be72 into main May 4, 2023
@ktlim ktlim deleted the tickets/DM-38269 branch May 4, 2023 00:48
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.

4 participants