Conversation
There was a problem hiding this comment.
Pull request overview
Fixes and enables the IMAP spacecraft spacecraft/l1a/pointing-attitude processing job by ensuring it can be triggered from its non-science inputs and by introducing a custom dynamic partitioning scheme aligned to attitude-history (AH) kernel coverage.
Changes:
- Add
repointdependency handling in the kickoff sensor and fix an unconditional “missing science inputs” guard for jobs that have no science inputs. - Introduce a new
pointing_attitudedynamic partition definition + sensor to maintain partitions based on AH kernel coverage mapped onto pointing times. - Update the spacecraft job dependency config to use the new partition type, and adjust pytest options to avoid the
anyioplugin incompatibility.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| sds_data_manager/orchestration/imap_job.py | Adds repoint dependency triggering and fixes science-input handling for non-science jobs. |
| sds_data_manager/orchestration/dependencies/imap_spacecraft_dependencies.yaml | Switches pointing-attitude from daily to pointing_attitude partitions. |
| sds_data_manager/orchestration/custom_partitions.py | Adds the pointing_attitude_partitions dynamic partition + sensor and registers it. |
| pyproject.toml | Disables the anyio pytest plugin to keep pytest 6.x collection working. |
Comments suppressed due to low confidence (1)
sds_data_manager/orchestration/imap_job.py:843
- This error message is now triggered only when
science_inputsare configured, but it still says “All jobs require at least one science file.” That’s misleading (and contradicts the existence of non-science-only jobs like pointing-attitude). Update the message to reflect the actual condition: science inputs were configured/required but none were found.
raise MissingDependenciesError(
"No science files were discovered. "
"All jobs require at least one science file."
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
00dff3f to
a71953e
Compare
52aac62 to
5b820b5
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new pointing-attitude partition sensor has a verified inconsistency between its “fully covered” pointing filter and the partition end timestamp, which can generate partitions that claim uncovered time ranges.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
sds_data_manager/orchestration/custom_partitions.py:420
last_coveredis constrained byrepoint_start_utc <= ah_max, but the partition end is taken frompointing_end_utc. In the pointing table,pointing_end_utcis derived from the next row’srepoint_end_utc(seespice_indexer.index_pointing_data), so this can produce a partition end timestamp that extends beyond the attitude-history kernel coverage window implied by the filter. Align the containment filter with the timestamp used in the partition key to avoid generating partitions that claim uncovered time.
# Last pointing completely contained within the ah kernel coverage
last_covered = (
session.query(models.PointingTable)
.filter(
models.PointingTable.pointing_start_utc >= ah_min,
models.PointingTable.repoint_start_utc <= ah_max,
)
.order_by(models.PointingTable.pointing_end_utc.desc())
.first()
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
jaredclaypoole
left a comment
There was a problem hiding this comment.
A few small things, otherwise this looks good.
8bfa1a0 to
21dd20c
Compare
|
Thanks so much for this! I think this should really help with cutting down on assets getting triggered. Not to mention it is just a way more accurate way of viewing the pointing_attitude data. Should we also modify sds_data_manager/orchestration/custom_behavior/spacecraft.py? Right now I think I set it so the spacecraft jobs trigger on new repoint partitions being created. But now that we have this new partition, should we instead trigger a job based on any new pointing_attitude partitions? I'm just thinking of the scenario where a new repoint partition is added before a pointing_attitude partition, and then the spacecraft_attitude job is triggered, all before the newer pointing_attitude partitions are made. |
b6c0dde to
6ea42e6
Compare
Add test coverage
PR feedback
…aterialized Override pointing_attitude versioning
0736411 to
f81db17
Compare
…ed ah files arrive
Fix and enable spacecraft
pointing-attitudeprocessing jobBackground
The
spacecraft/l1a/pointing-attitudejob produces a SPICE kernel encodingdespun spacecraft attitude for each pointing period. Its inputs are an
attitude_history(ah) SPICE kernel and arepointfile; it has no sciencedata inputs.
The ah kernel has an unusual delivery pattern. Each new delivery appends
coverage to the previous file, growing the covered time range up to
approximately three months before resetting. Early in the mission, before the
appending scheme was adopted, ah files covered approximately one day each.
When conops changed, the team retroactively produced a single combined file
covering the first ~three months. Because
spice_files.file_nameis unique,none of those superseded files are ever deleted — they remain in the DB
alongside the kernel(s) that replaced them.
Because one run of the pointing-attitude job corresponds to the full coverage
of one ah kernel cycle — not a calendar day — the partition and sensor logic
require special handling compared to other processing jobs.
Changes
Bug fixes in
imap_job.py1. Science-input guard was unconditional
get_science_files_inputsraisedMissingDependenciesError("All jobs require at least one science file.")whenever the collected science inputs were empty — even when the job was not
configured with any science inputs at all. The pointing-attitude job has only
SPICE and repoint inputs, so it hit this guard on every run. Fixed by adding
and self.job_config.science_inputsto the condition, so the error is onlyraised when science inputs were expected but not found.
2. Sensor had no handler for
repointdependency typeThe
build_sensorloop dispatches ondependency.data_typeto determinewhich table to query for new files. The supported types were
VALID_DATALEVELS,spice,spin, andancillary. There was norepointcase, so new repoint files never produced any
target_partitionsand nevertriggered the pointing-attitude job. Added an
elif dependency.data_type == "repoint":branch callingtrigger_from_new_non_science_inputswithmodels.RepointFiles.3.
RepointFileshas nostart_datecolumntrigger_from_new_non_science_inputsdefaults to readingstart_dateandend_datefrom each file record.RepointFilesonly hasend_date. Withoutexplicit column overrides, this would raise
AttributeErroron the secondsensor run (after the cursor advances past
MISSION_START_TIME). The repointbranch passes
datetime_start_column="end_date"anddatetime_end_column="end_date"to avoid this.New custom partition:
pointing_attitude(custom_partitions.py)dailypartitions were previously configured for this job, meaning thesensor would attempt to run once per calendar day. This was wrong: the output
of each job run covers the full time range of the ah kernel (potentially
months), not a single day. One run corresponds to one ah kernel cycle.
The new
pointing_attitude_partitions(DynamicPartitionsDefinition)creates one partition per ah kernel, with the key encoding the pointing
times covered — not the raw kernel timestamps:
pointing_start_utcof the first pointing with any overlap with the ah kernel's coveragepointing_end_utcof the last pointing completely contained within the ah kernel's coverageUsing pointing times (rather than kernel timestamps) aligns the partition
boundaries exactly with the science data being processed and ensures no
partially-covered pointing is claimed as complete.
imap_spacecraft_dependencies.yaml's(l1a, pointing-attitude)entry isupdated to
partition: pointing_attitude(wasrepoint), wiring the job tothis new partition set instead of the repoint-keyed one. The trigger for the
job remains the
repointfile, since it's delivered to the SDC after theupdated ah kernel and is therefore the reliable signal that all inputs are
ready — only the partitioning changed, not the trigger.
Partition lifecycle — subsumption-based deletion
The
add_pointing_attitude_partitionssensor handles two update scenarios bydeleting any existing partition whose full range is contained within the new
partition's range before adding the new one:
Superseded kernels are filtered out before partitions are generated
Because old ah kernel rows are never deleted, the sensor previously kept
regenerating partitions for kernels whose coverage was fully contained within
a newer combined/append kernel — undoing the subsumption-based deletion above
(a partition gets deleted as subsumed, then immediately re-added because its
source kernel is still in the DB with no matching partition). A new
_select_maximal_ah_kernelshelper filtersattitude_kernelsdown to onlythose not fully contained within another kernel's coverage before any
partitions are computed.
Kernels are sorted by coverage duration (longest first) so each candidate is
only compared against the kernels already accepted as maximal — that list
stays small in practice (one entry per "generation" of combined files),
keeping this cheap even on a cold start against a hundred-plus kernels. As a
side effect,
partitions_to_addnow lists larger-duration partitions first.Partition key prefix
The prefix
pointingattitude(no underscores) is required becauseparse_dates_from_partition_keysplits on the first_to isolate the daterange portion of the key. A multi-word prefix with underscores would leave
part of the prefix attached to the start datetime string, causing
strptimeto fail.
Tests (
tests/orchestration/test_custom_partitions.py)Nine unit tests for
add_pointing_attitude_partitions, added to the existingtest_custom_partitions.py(alongside the idex/cadence sensor tests alreadythere) rather than a standalone file:
first_overlappingexists butlast_coveredisNonepyproject.tomlAdded
-p no:anyiotoaddopts. Installing thecdk-installdependencygroup (which includes
dagster) also pulls inanyio, which registers apytest plugin requiring pytest ≥7. The project uses pytest 6.2.5. Disabling
the plugin via
-p no:anyiorestores normal test collection withoutrequiring a pytest upgrade.