First pass at PDS4 bundle generation #68
Conversation
WalkthroughAdds PDS4 bundle-generation (labels/summary) and Cloud Tasks drivers, expands DataSet PDS4 APIs (and Cassini ISS Cruise/Saturn subclasses), and refactors backplane generation to exception-driven flow with changed return shapes and metadata outputs; many new PDS4 templates and CLI entry points included. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #68 +/- ##
==========================================
- Coverage 36.98% 35.90% -1.09%
==========================================
Files 57 58 +1
Lines 4350 4726 +376
Branches 564 597 +33
==========================================
+ Hits 1609 1697 +88
- Misses 2641 2929 +288
Partials 100 100 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 55
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backplanes/merge.py (1)
22-24: Docstring is inconsistent with the new return signature.The docstring on line 23 states "A tuple containing the master by type, body id map, and merge info" but the function now returns only two values. Update the docstring to match the new signature.
Suggested fix
Returns: - A tuple containing the master by type, body id map, and merge info. + A tuple containing (master_by_type, body_id_map).
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
src/pds4/templates/cassini_iss_saturn_1.0/collection_context.csvis excluded by!**/*.csvsrc/pds4/templates/cassini_iss_saturn_1.0/collection_document.csvis excluded by!**/*.csvsrc/pds4/templates/cassini_iss_saturn_1.0/collection_xml_schema.csvis excluded by!**/*.csv
📒 Files selected for processing (48)
README.mddocs/developer_guide_class_hierarchy.rstdocs/developer_guide_extending.rstdocs/index.rstdocs/user_guide_navigation.rstdocs/user_guide_pds4_bundle.rstpyproject.tomlsrc/backplanes/backplanes.pysrc/backplanes/backplanes_bodies.pysrc/backplanes/backplanes_rings.pysrc/backplanes/merge.pysrc/backplanes/templates/backplanes.lblxsrc/backplanes/writer.pysrc/experiments/backplanes/smoke_backplanes_sim.pysrc/main/nav_backplane_viewer.pysrc/main/nav_backplanes.pysrc/main/nav_backplanes_cloud_tasks.pysrc/main/nav_create_bundle.pysrc/main/nav_create_bundle_cloud_tasks.pysrc/main/nav_offset.pysrc/main/nav_offset_cloud_tasks.pysrc/nav/config/config.pysrc/nav/config_files/config_95_pds4.yamlsrc/nav/dataset/__init__.pysrc/nav/dataset/dataset.pysrc/nav/dataset/dataset_pds3.pysrc/nav/dataset/dataset_pds3_cassini_iss.pysrc/nav/dataset/dataset_pds3_galileo_ssi.pysrc/nav/dataset/dataset_pds3_newhorizons_lorri.pysrc/nav/dataset/dataset_pds3_voyager_iss.pysrc/nav/dataset/dataset_sim.pysrc/nav/navigate_image_files.pysrc/pds4/__init__.pysrc/pds4/bundle_data.pysrc/pds4/collections.pysrc/pds4/templates/cassini_iss_saturn_1.0/browse.lblxsrc/pds4/templates/cassini_iss_saturn_1.0/bundle.lblxsrc/pds4/templates/cassini_iss_saturn_1.0/cassini-iss-saturn-backplanes-user-guide.lblxsrc/pds4/templates/cassini_iss_saturn_1.0/collection_browse.lblxsrc/pds4/templates/cassini_iss_saturn_1.0/collection_context.lblxsrc/pds4/templates/cassini_iss_saturn_1.0/collection_data.lblxsrc/pds4/templates/cassini_iss_saturn_1.0/collection_document.lblxsrc/pds4/templates/cassini_iss_saturn_1.0/collection_xml_schema.lblxsrc/pds4/templates/cassini_iss_saturn_1.0/data.lblxsrc/pds4/templates/cassini_iss_saturn_1.0/data_orig.lblxsrc/pds4/templates/cassini_iss_saturn_1.0/global_index_bodies.lblxsrc/pds4/templates/cassini_iss_saturn_1.0/global_index_rings.lblxsrc/pds4/templates/cassini_iss_saturn_1.0/readme.txt
💤 Files with no reviewable changes (1)
- src/backplanes/templates/backplanes.lblx
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-11-24T19:48:51.676Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 48
File: main/nav_main_offset.py:167-171
Timestamp: 2025-11-24T19:48:51.676Z
Learning: In the nav_main_offset.py file, it is normal and expected for the nav_default_config.yaml file to be missing. The FileNotFoundError suppression when attempting to load this default config is intentional behavior, not an error condition that needs logging.
Applied to files:
pyproject.tomlsrc/main/nav_offset.pysrc/main/nav_backplanes.py
📚 Learning: 2025-09-22T21:03:10.431Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 1
File: tests/nav/dataset/test_dataset_pds3_newhorizons_lorri.py:27-33
Timestamp: 2025-09-22T21:03:10.431Z
Learning: PDS3 (Planetary Data System version 3) archives are immutable and never updated once finalized. Exact count assertions in tests for PDS3 datasets are appropriate since the underlying data will never change.
Applied to files:
src/nav/dataset/dataset_pds3.pysrc/nav/dataset/dataset_pds3_voyager_iss.pysrc/nav/dataset/dataset_pds3_galileo_ssi.pysrc/nav/dataset/dataset.pysrc/nav/dataset/dataset_pds3_newhorizons_lorri.pysrc/nav/dataset/dataset_pds3_cassini_iss.pydocs/developer_guide_class_hierarchy.rst
📚 Learning: 2025-09-22T23:18:15.371Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 1
File: tests/nav/dataset/test_dataset_pds3_cassini_iss.py:29-34
Timestamp: 2025-09-22T23:18:15.371Z
Learning: PDS3 dataset indices are stable and immutable once archived, making exact count assertions appropriate in tests rather than using lower-bound checks.
Applied to files:
src/nav/dataset/dataset_pds3.py
📚 Learning: 2025-12-04T23:41:21.874Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 52
File: nav/obs/obs_inst_voyager_iss.py:47-49
Timestamp: 2025-12-04T23:41:21.874Z
Learning: In the rms-nav codebase, `FCPath.get_local_path()` from the filecache library returns `Path | list[Path]`. When used with a single path argument, `cast(Path, fc_path.get_local_path())` is required to narrow the type to `Path` for type checking purposes. This pattern is intentionally used across observation instrument files (obs_inst_voyager_iss.py, obs_inst_newhorizons_lorri.py, obs_inst_cassini_iss.py, obs_inst_galileo_ssi.py).
Applied to files:
src/nav/dataset/dataset_pds3_galileo_ssi.pysrc/backplanes/backplanes.py
📚 Learning: 2025-11-24T20:38:55.660Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 48
File: nav/backplanes/backplanes_bodies.py:90-97
Timestamp: 2025-11-24T20:38:55.660Z
Learning: In nav/backplanes/backplanes_bodies.py, the hash-based RNG seeding for simulated backplanes (using hash((body_name, name))) is acceptable and reproducibility across Python interpreter runs is not required for simulated mode.
Applied to files:
src/backplanes/backplanes_bodies.py
📚 Learning: 2025-09-22T21:16:27.113Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 1
File: docs/user_guide.rst:78-92
Timestamp: 2025-09-22T21:16:27.113Z
Learning: PdsTable uses the .lbl file to automatically locate and read the corresponding .tab file in PDS3 datasets. Code should return .lbl file paths only, as PdsTable handles finding and reading the .tab file internally. Both .lbl and .tab files exist in the PDS3 structure but only .lbl paths need to be specified in volume_to_index implementations.
Applied to files:
src/nav/dataset/dataset_pds3_cassini_iss.py
🧬 Code graph analysis (13)
src/nav/config/config.py (1)
src/nav/support/attrdict.py (1)
AttrDict(5-29)
src/nav/navigate_image_files.py (1)
src/nav/support/nav_base.py (1)
logger(38-40)
src/main/nav_create_bundle_cloud_tasks.py (4)
src/nav/dataset/dataset.py (2)
ImageFile(15-58)ImageFiles(62-77)src/nav/dataset/__init__.py (1)
dataset_name_to_class(57-69)src/nav/config/config.py (4)
pds4(227-231)read_config(60-81)update_config(83-102)environment(119-123)src/pds4/bundle_data.py (1)
generate_bundle_data_files(14-131)
src/backplanes/writer.py (3)
src/nav/support/file.py (1)
json_as_string(94-102)src/nav/obs/obs_snapshot.py (1)
closest_planet(302-303)src/nav/config/config.py (1)
satellites(132-145)
src/pds4/__init__.py (2)
src/pds4/bundle_data.py (1)
generate_bundle_data_files(14-131)src/pds4/collections.py (2)
generate_collection_files(13-126)generate_global_index_files(129-322)
src/main/nav_offset_cloud_tasks.py (2)
src/nav/dataset/__init__.py (1)
dataset_name_to_inst_name(72-84)src/nav/obs/__init__.py (1)
inst_name_to_obs_class(26-35)
src/main/nav_offset.py (1)
src/nav/dataset/__init__.py (2)
dataset_names(52-54)dataset_name_to_class(57-69)
src/main/nav_create_bundle.py (3)
src/nav/dataset/__init__.py (2)
dataset_names(52-54)dataset_name_to_class(57-69)src/pds4/bundle_data.py (1)
generate_bundle_data_files(14-131)src/pds4/collections.py (2)
generate_collection_files(13-126)generate_global_index_files(129-322)
src/nav/dataset/__init__.py (1)
src/nav/dataset/dataset_pds3_cassini_iss.py (3)
DataSetPDS3CassiniISS(14-621)DataSetPDS3CassiniISSCruise(624-638)DataSetPDS3CassiniISSSaturn(641-655)
src/nav/dataset/dataset.py (5)
src/nav/dataset/dataset_pds3_cassini_iss.py (9)
pds4_bundle_template_dir(311-333)pds4_bundle_name(335-346)pds4_bundle_path_for_image(349-368)pds4_path_stub(370-384)pds4_image_name_to_browse_lid(386-397)pds4_image_name_to_browse_lidvid(399-410)pds4_image_name_to_data_lid(412-423)pds4_image_name_to_data_lidvid(425-436)pds4_template_variables(438-597)src/nav/dataset/dataset_pds3_newhorizons_lorri.py (5)
pds4_bundle_template_dir(182-197)pds4_bundle_name(199-204)pds4_bundle_path_for_image(207-209)pds4_path_stub(211-213)pds4_template_variables(215-222)src/nav/dataset/dataset_sim.py (5)
pds4_bundle_template_dir(65-67)pds4_bundle_name(69-71)pds4_bundle_path_for_image(74-76)pds4_path_stub(78-80)pds4_template_variables(82-89)src/nav/dataset/dataset_pds3_galileo_ssi.py (5)
pds4_bundle_template_dir(198-213)pds4_bundle_name(215-220)pds4_bundle_path_for_image(223-225)pds4_path_stub(227-229)pds4_template_variables(231-238)src/nav/dataset/dataset_pds3_voyager_iss.py (5)
pds4_bundle_template_dir(189-204)pds4_bundle_name(206-211)pds4_bundle_path_for_image(214-216)pds4_path_stub(218-220)pds4_template_variables(222-229)
src/main/nav_backplanes_cloud_tasks.py (4)
src/nav/dataset/__init__.py (1)
dataset_name_to_inst_name(72-84)src/nav/obs/__init__.py (1)
inst_name_to_obs_class(26-35)src/backplanes/backplanes.py (1)
generate_backplanes_image_files(18-102)src/nav/dataset/dataset.py (1)
ImageFiles(62-77)
src/main/nav_backplanes.py (2)
src/nav/dataset/__init__.py (2)
dataset_names(52-54)dataset_name_to_class(57-69)src/backplanes/backplanes.py (1)
generate_backplanes_image_files(18-102)
src/nav/dataset/dataset_pds3_cassini_iss.py (1)
src/nav/dataset/dataset_pds3_galileo_ssi.py (5)
pds4_bundle_template_dir(198-213)pds4_bundle_name(215-220)pds4_bundle_path_for_image(223-225)pds4_path_stub(227-229)pds4_template_variables(231-238)
🪛 Ruff (0.14.10)
src/pds4/bundle_data.py
34-35: Avoid specifying long messages outside the exception class
(TRY003)
45-45: Use explicit conversion flag
Replace with conversion flag
(RUF010)
107-107: Avoid specifying long messages outside the exception class
(TRY003)
src/nav/config/config.py
227-227: Dynamically typed expressions (typing.Any) are disallowed in pds4
(ANN401)
src/nav/navigate_image_files.py
64-64: Redundant exception object included in logging.exception call
(TRY401)
75-75: Redundant exception object included in logging.exception call
(TRY401)
src/main/nav_create_bundle_cloud_tasks.py
31-31: Unused function argument: task_id
(ARG001)
src/backplanes/backplanes_rings.py
53-53: Avoid specifying long messages outside the exception class
(TRY003)
70-70: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Avoid specifying long messages outside the exception class
(TRY003)
75-75: Logging statement uses f-string
(G004)
src/pds4/collections.py
36-36: Avoid specifying long messages outside the exception class
(TRY003)
src/nav/dataset/dataset_pds3_voyager_iss.py
224-224: Unused method argument: image_file
(ARG002)
225-225: Unused method argument: nav_metadata
(ARG002)
226-226: Unused method argument: backplane_metadata
(ARG002)
src/nav/dataset/dataset_pds3_galileo_ssi.py
233-233: Unused method argument: image_file
(ARG002)
234-234: Unused method argument: nav_metadata
(ARG002)
235-235: Unused method argument: backplane_metadata
(ARG002)
src/backplanes/writer.py
88-89: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
91-91: Do not catch blind exception: Exception
(BLE001)
src/backplanes/backplanes_bodies.py
54-54: Do not catch blind exception: Exception
(BLE001)
114-114: Avoid specifying long messages outside the exception class
(TRY003)
144-144: Avoid specifying long messages outside the exception class
(TRY003)
147-147: Avoid specifying long messages outside the exception class
(TRY003)
149-149: Logging statement uses f-string
(G004)
src/main/nav_create_bundle.py
38-38: Boolean-typed positional argument in function definition
(FBT001)
38-38: Boolean default positional argument in function definition
(FBT002)
156-158: Avoid specifying long messages outside the exception class
(TRY003)
188-190: Avoid specifying long messages outside the exception class
(TRY003)
202-204: Avoid specifying long messages outside the exception class
(TRY003)
src/main/nav_backplane_viewer.py
1450-1452: Avoid specifying long messages outside the exception class
(TRY003)
1464-1466: Avoid specifying long messages outside the exception class
(TRY003)
src/main/nav_backplanes.py
182-184: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/backplanes/backplanes.py
43-44: Avoid specifying long messages outside the exception class
(TRY003)
51-51: Use explicit conversion flag
Replace with conversion flag
(RUF010)
67-67: Prefer TypeError exception for invalid type
(TRY004)
67-67: Avoid specifying long messages outside the exception class
(TRY003)
72-72: Avoid specifying long messages outside the exception class
(TRY003)
74-74: Logging statement uses f-string
(G004)
src/nav/dataset/dataset_pds3_cassini_iss.py
443-443: Unused method argument: backplane_metadata
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test nav (ubuntu-latest, 3.10)
- GitHub Check: Test nav (ubuntu-latest, 3.11)
- GitHub Check: Test nav (ubuntu-latest, 3.13)
- GitHub Check: Test nav (ubuntu-latest, 3.12)
🔇 Additional comments (52)
src/nav/config_files/config_95_pds4.yaml (1)
1-18: LGTM! Configuration structure is clear and extensible.The YAML structure is well-organized with commented examples for future datasets. Consider whether the hardcoded year in
bundle_name(rsfrench2027) should be parameterized or documented as intentional.src/main/nav_backplane_viewer.py (2)
1448-1453: Good three-tier configuration fallback pattern.The CLI → config → environment variable resolution order is clear and the error message helpfully enumerates all sources checked. The switch to
FileCache()is consistent with other changes in this PR.
1462-1467: Consistent pattern with nav_results_root handling.Same three-tier fallback approach applied correctly for backplane results root.
src/backplanes/backplanes_bodies.py (1)
12-62: Helper function looks correct.The deterministic seeding via
hash((body_name, backplane_name))is acceptable per previous learnings. The fallback chain (mask_map → sim_body_index_map → full rect) provides robust handling for different simulation scenarios.README.md (2)
36-37: Good feature documentation additions.New features clearly describe the PDS4 bundle generation and backplane generation capabilities.
101-118: Clear and consistent Quick Start examples.The new CLI examples follow the established pattern and demonstrate the workflow progression: navigation → backplanes → bundle generation. The
coiss_saturndataset name aligns with the new Cassini ISS Saturn dataset variant introduced in this PR.docs/index.rst (1)
16-16: LGTM! Theuser_guide_pds4_bundleentry is correctly positioned in the toctree hierarchy, and the referenced documentation file exists.src/nav/dataset/dataset_pds3.py (1)
57-61: Clarify intent of removing FileCache namespace for holdings cache.The
pds3_holdings_filecacheno longer uses a named cache key (changed fromFileCache('nav_pds3_holdings')toFileCache()), whileindex_filecacheretains its namespace asFileCache('nav_pds3_index'). If this standardizes cache handling across the PR, confirm both should follow the same pattern. If holdings uses temporary caching while index uses persistent caching, the design difference should be documented.src/nav/config/config.py (1)
29-29: LGTM: PDS4 configuration section follows established pattern.The new PDS4 configuration section is implemented consistently with existing configuration sections (environment, general, backplanes, etc.). The three-part pattern (initialization in
__init__, population in_update_attrdicts, and property accessor) is correctly applied.Note: The static analysis warning about
Anytype hint is consistent with all other config property accessors in this file and can be safely ignored.Also applies to: 46-46, 226-231
src/experiments/backplanes/smoke_backplanes_sim.py (1)
19-19: LGTM: FileCache initialization updated for consistency.The change from
FileCache('backplane_results')toFileCache()aligns with the broader refactoring pattern across the codebase to standardize root path resolution. Paths are still correctly derived viafc.new_path().src/main/nav_offset.py (2)
57-57: LGTM: CLI usage messages updated for consistency.The usage messages now correctly reference the installed entry point name
nav_main_offsetinstead of the internal script path, improving user experience.Also applies to: 65-65, 73-73
191-191: LGTM: FileCache initialization standardized.The change from
FileCache('nav_results').new_path(...)toFileCache().new_path(...)aligns with the broader refactoring to remove dataset-specific cache namespaces, consistent with changes in nav_backplanes.py and other modules.src/pds4/templates/cassini_iss_saturn_1.0/collection_xml_schema.lblx (1)
57-74: Verify contributor sequence_number for Mitchell K. Gordon.Line 63 sets
sequence_numberto 2 for Mitchell K. Gordon, which is correct as he is the second contributor in the List_Contributor. However, please verify this is intentional, as contributors appear in a specific order and sequence numbers should match.docs/developer_guide_extending.rst (1)
32-56: LGTM: Clear documentation for PDS4 bundle generation extension points.The new section clearly documents the five required abstract methods for PDS4 bundle support, including their purpose and expected behavior. The note about raising
NotImplementedErrorfor unsupported datasets is helpful, and the reference to a complete implementation example is valuable.pyproject.toml (1)
77-77: LGTM: Entry points updated for PDS4 bundle generation.The changes add two new CLI entry points (
nav_create_bundleandnav_create_bundle_cloud_tasks) and standardize the path format fornav_backplanesto be consistent with other entry points. All now use themain.module:mainpattern.Also applies to: 80-81
src/pds4/templates/cassini_iss_saturn_1.0/browse.lblx (1)
1-70: LGTM: Well-structured PDS4 browse template.The Product_Browse template is cleanly structured with proper identification, references, and file area sections. Template placeholders follow the established pattern for dynamic value substitution during bundle generation.
docs/user_guide_navigation.rst (2)
33-35: LGTM! Clear documentation of Cassini ISS variants.The documentation clearly distinguishes between the three Cassini ISS variants and their volume coverage, which will help users select the appropriate dataset for their needs.
223-223: LGTM! Dataset list updated consistently.The updated dataset selection documentation properly includes all new Cassini ISS variants (coiss_cruise, coiss_cruise_pds3, coiss_saturn, coiss_saturn_pds3) alongside the existing datasets.
src/pds4/templates/cassini_iss_saturn_1.0/collection_data.lblx (1)
191-223: LGTM! File_Area_Inventory structure is correct.The inventory structure properly defines the CSV file metadata with appropriate placeholders for dynamic values (creation time, size, checksum, record count). The field definitions for Member Status and LIDVID_LID follow PDS4 standards.
src/pds4/__init__.py (1)
1-10: LGTM! Clean public API exposure.The module initialization properly exposes the three main PDS4 bundle generation functions through
__all__, establishing a clear public API surface. The imports are correctly structured using relative imports.src/pds4/templates/cassini_iss_saturn_1.0/collection_browse.lblx (1)
129-161: LGTM! Consistent File_Area_Inventory structure.The browse inventory structure is consistent with the data collection template and properly defines the CSV file metadata with appropriate placeholders.
src/pds4/templates/cassini_iss_saturn_1.0/data_orig.lblx (3)
36-36: Verify publication_year for backplanes product.The publication_year is set to 2007, which may be appropriate for the original Cassini ISS data. However, since this is a template for backplanes derived products (as indicated by the keywords and the bundle name), the publication year should likely be the year of the backplanes product release (2027, as used in the collection templates). Please verify which year is appropriate.
176-185: TODO: Determine SPICE kernel handling strategy.The comment indicates uncertainty about how to handle SPICE kernels in the dataset. This is a significant metadata requirement that should be resolved before the template is finalized.
Would you like me to open an issue to track this TODO and gather requirements for SPICE kernel documentation in PDS4 products?
226-226: TODO: Verify reference_type for source product.The reference_type
data_to_calibrated_source_productshould be verified against PDS4 standards to ensure it's the correct relationship type for navigated backplanes derived from calibrated images.src/main/nav_backplanes_cloud_tasks.py (3)
55-74: LGTM! Standardized error handling and FileCache usage.The changes improve consistency by:
- Returning structured error payloads with
statusandstatus_errorfields instead of plain strings- Simplifying FileCache usage to a generic
FileCache()initializationThese changes align with error handling patterns across the cloud-tasks infrastructure.
76-117: LGTM! Comprehensive validation with structured error returns.The validation logic properly checks all required fields and returns consistent error payloads. The addition of
status_exceptionfor the unknown dataset error (line 88) provides valuable debugging context.
126-134: Review exception handling expectations for generate_backplanes_image_files.The function returns
(False, None)unconditionally without exception handling aroundgenerate_backplanes_image_files. If that call raises an exception, it will propagate uncaught. Clarify whether:
- The cloud tasks worker framework is expected to handle uncaught exceptions and respect the
Falsereturn value for no-retry behavior, or- Exceptions should be caught and converted to structured error responses for consistency with other handlers
src/main/nav_offset_cloud_tasks.py (2)
77-106: Structured error payloads improve error handling consistency.The standardized error dictionaries with
statusandstatus_errorkeys provide a consistent API for consumers of task results. This aligns with the broader pattern introduced across related cloud-task modules.
139-145: No action required. The metadata returned bynavigate_image_filesfollows the same structured format as other error payloads in the codebase, with consistent'status'and'status_error'keys, ensuring compatible downstream processing.Likely an incorrect or invalid review comment.
docs/user_guide_pds4_bundle.rst (1)
1-63: Comprehensive documentation for PDS4 bundle generation.The documentation provides clear explanations of the two-pass bundle generation process, bundle structure, and directory organization. The workflow is well-documented and aligns with the implementation in the CLI and cloud-task modules.
src/pds4/templates/cassini_iss_saturn_1.0/cassini-iss-saturn-backplanes-user-guide.lblx (1)
163-163: TODO: Target context references need to be added.The comment
<!-- TODO Need to add targets here -->indicates missing target context references in the Context_Area. This should be addressed to provide complete PDS4 compliance.src/nav/dataset/dataset_sim.py (1)
65-89: Placeholder PDS4 methods correctly indicate unsupported functionality.The five new PDS4-related methods appropriately raise
NotImplementedErrorwith a consistent message. This pattern aligns with the baseDataSetinterface and clearly indicates that PDS4 bundle generation is not supported for simulated datasets.src/nav/dataset/__init__.py (1)
2-6: Dataset registry correctly extended for new Cassini ISS variants.The new
DataSetPDS3CassiniISSCruiseandDataSetPDS3CassiniISSSaturnclasses are properly imported, mapped with both standard and_pds3suffixed names, and exported in__all__. The instrument name mappings correctly reference'coiss'for all variants.Also applies to: 16-19, 33-36, 89-90
src/backplanes/merge.py (2)
47-54: Good defensive handling for simulated snapshot NAIF IDs.The fallback logic that generates deterministic fake NAIF IDs for simulated snapshots with unknown body names is a sensible approach that prevents crashes during testing while still raising errors for real data issues.
14-14: Breaking API change: verify all callers are updated.The function now returns 2 values instead of 3, and expects
bodies_resultto be a dict keyed by body name (iterated via.items()) rather than containing a'per_body'key. Ensure all callers ofmerge_sources_into_masterare updated accordingly.#!/bin/bash # Find all usages of merge_sources_into_master to verify they handle the new return signature rg -n "merge_sources_into_master" --type py -A 3Also applies to: 34-34
src/pds4/templates/cassini_iss_saturn_1.0/data.lblx (2)
1-25: PDS4 template structure follows current schema versions.The template correctly references PDS4 schema versions (1O00 series) and includes appropriate namespace declarations for rings, display, geometry, and Cassini mission-specific metadata.
93-99: Multiple TODOs for targets and ISS-specific attributes need tracking.Lines 93, 97-98 indicate that target context and ISS-specific attributes are not yet implemented. These are important for complete PDS4 metadata. Consider creating issues to track these remaining items.
src/pds4/bundle_data.py (1)
114-122: Address inefficiency in summary PNG copying.The current implementation works with cloud storage through filecache's local caching mechanism, but the TODO correctly notes this approach could be optimized. Rather than downloading a file, copying locally, then uploading, consider whether filecache or the destination FCPath object provides a direct copy method to avoid the intermediate local operations. If no such method exists, the current pattern remains the standard approach used throughout the codebase.
src/nav/dataset/dataset_pds3_galileo_ssi.py (1)
198-238: PDS4 method stubs are consistent with other datasets.The placeholder implementations for
pds4_bundle_path_for_image,pds4_path_stub, andpds4_template_variablesfollow the same pattern as other dataset classes. The unused arguments flagged by static analysis are expected for interface conformance.Consider adding a TODO comment or docstring noting these require implementation for full PDS4 support.
src/main/nav_backplanes.py (1)
154-160: Good addition of startup logging for operational visibility.The verbose startup logging provides useful diagnostic information for debugging and operational monitoring.
src/nav/dataset/dataset_pds3_voyager_iss.py (1)
189-229: PDS4 method stubs follow the established pattern.The implementation is consistent with other dataset classes (Galileo SSI, New Horizons LORRI). The unused arguments are expected for interface conformance with the base
DataSetclass.src/backplanes/backplanes.py (1)
81-101: Clean refactoring to exception-based error handling.The shift from status-dict returns to exception propagation simplifies the caller's error handling and aligns with Python best practices. The updated
write_fitscall signature withbodies_resultandrings_resultproperly passes context for metadata generation.src/nav/dataset/dataset_pds3_newhorizons_lorri.py (1)
182-204: PDS4 bundle configuration methods are correctly implemented.
pds4_bundle_template_dirandpds4_bundle_namefollow the same pattern as other dataset classes, with appropriate config lookups and defaults.src/backplanes/writer.py (2)
94-120: Well-structured per-body metadata extraction.The logic for extracting statistics and inventory data per body is clear. The coordinate swap from
[u, v]to[v, u]is properly documented with an inline comment.
72-73: The code assumes filenames always contain_backplanessuffix, but this assumption lacks validation.While the current call site always provides filenames with the
_backplanes.fitssuffix (constructed inbackplanes.pyline 49), thereplace('_backplanes', '')call at lines 72-73 silently does nothing if the filename doesn't contain this suffix. Consider validating the input or using a more explicit approach to derive the metadata filename, such as usingfits_file_path.stem[:-len('_backplanes')]with appropriate error handling.src/main/nav_create_bundle_cloud_tasks.py (1)
138-143: PassNonehandling toImageFile.index_file_row— avoid type mismatch with default factory.
index_file_rowis retrieved from task data via.get('index_file_row', None)(line 122) and can beNone. However, theImageFilefield is defined withfield(default_factory=dict), making it adict[str, Any]type. Explicitly passingNonediffers from omitting the parameter: the former sets the field toNone(violating the type annotation), while the latter invokes the default factory to set an empty dict. Either skip the parameter whenNoneor replace it with an empty dict before passing.src/nav/dataset/dataset.py (1)
152-280: Well-designed optional extension points for PDS4 support.The approach of using concrete methods with
NotImplementedErrorrather than abstract methods is a sound design choice. This allows existing datasets to continue working without PDS4 support while enabling new datasets to opt-in. The inline comments explaining this decision are helpful.src/main/nav_create_bundle.py (2)
162-233: LGTM for the main_labels implementation.The function correctly:
- Reads configuration with proper fallback to default config
- Derives roots with consistent priority (CLI > config > env)
- Validates single-image batches as required by
generate_bundle_data_files- Supports dry-run mode for testing
236-290: LGTM for the main_summary implementation.The function properly handles errors for both collection and index file generation with appropriate logging and exit codes.
docs/developer_guide_class_hierarchy.rst (2)
28-59: Documentation accurately reflects the new PDS4 extension points and Cassini ISS subclasses.The Mermaid class diagram correctly shows:
- PDS4 methods added to
DataSetandDataSetPDS3CassiniISS- New
DataSetPDS3CassiniISSCruiseandDataSetPDS3CassiniISSSaturnsubclasses- Proper inheritance relationships
Also applies to: 255-256
333-349: Documentation for Dataset section properly updated.The section now documents:
- PDS4 abstract methods in the base class
- New Cassini ISS subclasses with their volume ranges
- Updated dataset name mappings including
coiss_cruiseandcoiss_saturnsrc/nav/dataset/dataset_pds3_cassini_iss.py (1)
624-655: Well-structured subclass implementations for Cruise and Saturn datasets.The subclasses properly:
- Override
_ALL_VOLUME_NAMESto restrict to their respective volume ranges- Implement the three configuration methods (
_dataset_name_for_pds4_config,_default_pds4_template_dir,_default_pds4_bundle_name)- Inherit all other functionality from the parent class
|
@CodeRabbit summary |
✅ Actions performedSummary regeneration triggered. |
There was a problem hiding this comment.
Actionable comments posted: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/nav_backplanes_cloud_tasks.py (2)
31-33: Inconsistenttask_idparameter naming.In
nav_create_bundle_cloud_tasks.py, the parameter is_task_id(with underscore) to indicate intentional non-use. Here it'stask_idwithout underscore, though it's also unused. Consider aligning for consistency.🔎 Proposed fix
def process_task( - task_id: str, task_data: dict[str, Any], worker_data: WorkerData + _task_id: str, task_data: dict[str, Any], worker_data: WorkerData ) -> tuple[bool, Any]:
137-139: Confusing help text for--backplane-results-root.The help text says "overrides NAV_RESULTS_ROOT or config" but this argument is for backplane results, not navigation results. The environment variable should be
NAV_BACKPLANE_RESULTS_ROOT.🔎 Proposed fix
environment_group.add_argument( '--backplane-results-root', type=str, default=None, - help='Root directory for backplane results; overrides NAV_RESULTS_ROOT or config') + help='Root directory for backplane results; overrides NAV_BACKPLANE_RESULTS_ROOT or config')
♻️ Duplicate comments (5)
src/backplanes/backplanes_rings.py (3)
51-53: Error message is now correct.The error message "Configuration has no rings section for backplanes" correctly describes the condition when
rings_cfg is None.
71-73: Units validation now correctly implemented.The
unitsfield is now retrieved without a default value, allowing the None check to properly validate that units are required.
91-101: Statistics collection and assignment correctly implemented.The statistics dictionary is now properly populated inside the loop and assigned to
result['statistics']once after the loop completes (line 101). The unit conversion from radians to degrees is correctly applied before computing min/max.src/pds4/bundle_data.py (1)
111-126: Verify browse template existence before instantiation.Unlike the data template path (line 105-106), the browse template path at line 122-123 is not validated before creating
PdsTemplate. Ifbrowse.lblxdoesn't exist, an unclear error will occur.This was flagged in a previous review and hasn't been addressed yet. Consider adding an existence check similar to:
if not browse_template_path.exists(): logger.warning('Browse template not found: %s', browse_template_path) returnsrc/nav/dataset/dataset.py (1)
222-233: Docstring inconsistency: says "LID" but method returns "LIDVID".The docstring says "Returns the browse LID" but the method is
pds4_image_name_to_browse_lidvidwhich should return a LIDVID. The same issue exists forpds4_image_name_to_data_lidvid(lines 248-259).🔎 Proposed fix
def pds4_image_name_to_browse_lidvid(self, image_name: str) -> str: - """Returns the browse LIDVID for the given image name. + """Returns the browse LIDVID for the given image name. Parameters: - image_name: The image name to convert to a browse LID. + image_name: The image name to convert to a browse LIDVID. Returns: - The browse LID. + The browse LIDVID. """And for
pds4_image_name_to_data_lidvid:def pds4_image_name_to_data_lidvid(self, image_name: str) -> str: - """Returns the data LIDVID for the given image name. + """Returns the data LIDVID for the given image name. Parameters: - image_name: The image name to convert to a data LID. + image_name: The image name to convert to a data LIDVID. Returns: - The data LID. + The data LIDVID. """
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (27)
docs/developer_guide_class_hierarchy.rstdocs/developer_guide_extending.rstdocs/user_guide_pds4_bundle.rstsrc/backplanes/backplanes.pysrc/backplanes/backplanes_bodies.pysrc/backplanes/backplanes_rings.pysrc/backplanes/writer.pysrc/main/nav_backplane_viewer.pysrc/main/nav_backplanes.pysrc/main/nav_backplanes_cloud_tasks.pysrc/main/nav_create_bundle.pysrc/main/nav_create_bundle_cloud_tasks.pysrc/main/nav_offset.pysrc/main/nav_offset_cloud_tasks.pysrc/nav/config/__init__.pysrc/nav/config/config_helper.pysrc/nav/dataset/dataset.pysrc/nav/dataset/dataset_pds3_cassini_iss.pysrc/nav/dataset/dataset_pds3_galileo_ssi.pysrc/nav/dataset/dataset_pds3_newhorizons_lorri.pysrc/nav/dataset/dataset_pds3_voyager_iss.pysrc/pds4/bundle_data.pysrc/pds4/collections.pysrc/pds4/templates/cassini_iss_saturn_1.0/bundle.lblxsrc/pds4/templates/cassini_iss_saturn_1.0/collection_context.lblxsrc/pds4/templates/cassini_iss_saturn_1.0/collection_document.lblxsrc/pds4/templates/cassini_iss_saturn_1.0/readme.txt
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-24T19:48:51.676Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 48
File: main/nav_main_offset.py:167-171
Timestamp: 2025-11-24T19:48:51.676Z
Learning: In the nav_main_offset.py file, it is normal and expected for the nav_default_config.yaml file to be missing. The FileNotFoundError suppression when attempting to load this default config is intentional behavior, not an error condition that needs logging.
Applied to files:
src/main/nav_create_bundle_cloud_tasks.pysrc/main/nav_offset.py
📚 Learning: 2025-11-24T20:38:55.660Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 48
File: nav/backplanes/backplanes_bodies.py:90-97
Timestamp: 2025-11-24T20:38:55.660Z
Learning: In nav/backplanes/backplanes_bodies.py, the hash-based RNG seeding for simulated backplanes (using hash((body_name, name))) is acceptable and reproducibility across Python interpreter runs is not required for simulated mode.
Applied to files:
src/backplanes/backplanes_bodies.py
📚 Learning: 2025-09-22T21:03:10.431Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 1
File: tests/nav/dataset/test_dataset_pds3_newhorizons_lorri.py:27-33
Timestamp: 2025-09-22T21:03:10.431Z
Learning: PDS3 (Planetary Data System version 3) archives are immutable and never updated once finalized. Exact count assertions in tests for PDS3 datasets are appropriate since the underlying data will never change.
Applied to files:
src/nav/dataset/dataset_pds3_newhorizons_lorri.pysrc/nav/dataset/dataset_pds3_voyager_iss.pysrc/nav/dataset/dataset_pds3_cassini_iss.pydocs/developer_guide_class_hierarchy.rstsrc/nav/dataset/dataset_pds3_galileo_ssi.py
📚 Learning: 2025-12-04T23:41:21.874Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 52
File: nav/obs/obs_inst_voyager_iss.py:47-49
Timestamp: 2025-12-04T23:41:21.874Z
Learning: In the rms-nav codebase, `FCPath.get_local_path()` from the filecache library returns `Path | list[Path]`. When used with a single path argument, `cast(Path, fc_path.get_local_path())` is required to narrow the type to `Path` for type checking purposes. This pattern is intentionally used across observation instrument files (obs_inst_voyager_iss.py, obs_inst_newhorizons_lorri.py, obs_inst_cassini_iss.py, obs_inst_galileo_ssi.py).
Applied to files:
src/backplanes/backplanes.pysrc/nav/dataset/dataset_pds3_galileo_ssi.py
🧬 Code graph analysis (14)
src/main/nav_backplanes.py (2)
src/nav/config/config_helper.py (2)
get_backplane_results_root(7-38)get_nav_results_root(41-72)src/backplanes/backplanes.py (1)
generate_backplanes_image_files(18-99)
src/nav/config/__init__.py (1)
src/nav/config/config_helper.py (3)
get_backplane_results_root(7-38)get_nav_results_root(41-72)get_pds4_bundle_results_root(75-106)
src/nav/config/config_helper.py (1)
src/nav/config/config.py (2)
Config(9-231)environment(119-123)
src/main/nav_create_bundle_cloud_tasks.py (4)
src/nav/dataset/dataset.py (2)
ImageFile(15-60)ImageFiles(64-79)src/nav/dataset/__init__.py (1)
dataset_name_to_class(57-69)src/nav/config/config_helper.py (3)
get_backplane_results_root(7-38)get_nav_results_root(41-72)get_pds4_bundle_results_root(75-106)src/pds4/bundle_data.py (1)
generate_bundle_data_files(14-128)
src/backplanes/writer.py (3)
src/nav/support/file.py (1)
json_as_string(94-102)src/nav/obs/obs_snapshot.py (1)
closest_planet(302-303)src/nav/config/config.py (1)
satellites(132-145)
src/backplanes/backplanes_bodies.py (2)
src/nav/config/config.py (1)
backplanes(220-224)src/nav/support/nav_base.py (1)
logger(38-40)
src/pds4/collections.py (2)
src/nav/dataset/dataset.py (5)
pds4_bundle_name(170-180)pds4_bundle_template_dir(155-168)pds4_image_name_to_data_lidvid(248-259)pds4_image_name_to_browse_lidvid(222-233)pds4_path_stub(196-207)src/nav/dataset/dataset_pds3_cassini_iss.py (5)
pds4_bundle_name(335-346)pds4_bundle_template_dir(311-333)pds4_image_name_to_data_lidvid(425-436)pds4_image_name_to_browse_lidvid(399-410)pds4_path_stub(370-384)
src/main/nav_backplane_viewer.py (1)
src/nav/config/config_helper.py (2)
get_backplane_results_root(7-38)get_nav_results_root(41-72)
src/main/nav_offset.py (2)
src/nav/config/config_helper.py (1)
get_nav_results_root(41-72)src/nav/dataset/__init__.py (2)
dataset_names(52-54)dataset_name_to_class(57-69)
src/backplanes/backplanes_rings.py (3)
src/nav/config/config.py (1)
backplanes(220-224)src/nav/obs/obs_snapshot.py (2)
closest_planet(302-303)bp(332-338)src/nav/support/nav_base.py (1)
logger(38-40)
src/main/nav_offset_cloud_tasks.py (3)
src/nav/config/config_helper.py (1)
get_nav_results_root(41-72)src/nav/dataset/__init__.py (1)
dataset_name_to_inst_name(72-84)src/nav/obs/__init__.py (1)
inst_name_to_obs_class(26-35)
src/nav/dataset/dataset.py (5)
src/nav/dataset/dataset_pds3_cassini_iss.py (9)
pds4_bundle_template_dir(311-333)pds4_bundle_name(335-346)pds4_bundle_path_for_image(349-368)pds4_path_stub(370-384)pds4_image_name_to_browse_lid(386-397)pds4_image_name_to_browse_lidvid(399-410)pds4_image_name_to_data_lid(412-423)pds4_image_name_to_data_lidvid(425-436)pds4_template_variables(438-598)src/nav/dataset/dataset_pds3_galileo_ssi.py (9)
pds4_bundle_template_dir(198-213)pds4_bundle_name(215-220)pds4_bundle_path_for_image(223-225)pds4_path_stub(227-229)pds4_image_name_to_browse_lid(231-240)pds4_image_name_to_browse_lidvid(242-251)pds4_image_name_to_data_lid(253-262)pds4_image_name_to_data_lidvid(264-273)pds4_template_variables(275-282)src/nav/dataset/dataset_pds3_newhorizons_lorri.py (9)
pds4_bundle_template_dir(182-197)pds4_bundle_name(199-204)pds4_bundle_path_for_image(207-209)pds4_path_stub(211-213)pds4_image_name_to_browse_lid(215-224)pds4_image_name_to_browse_lidvid(226-235)pds4_image_name_to_data_lid(237-246)pds4_image_name_to_data_lidvid(248-257)pds4_template_variables(259-266)src/nav/dataset/dataset_pds3_voyager_iss.py (9)
pds4_bundle_template_dir(189-204)pds4_bundle_name(206-211)pds4_bundle_path_for_image(214-216)pds4_path_stub(218-220)pds4_image_name_to_browse_lid(222-231)pds4_image_name_to_browse_lidvid(233-242)pds4_image_name_to_data_lid(244-253)pds4_image_name_to_data_lidvid(255-264)pds4_template_variables(266-273)src/nav/dataset/dataset_sim.py (5)
pds4_bundle_template_dir(65-67)pds4_bundle_name(69-71)pds4_bundle_path_for_image(74-76)pds4_path_stub(78-80)pds4_template_variables(82-89)
src/nav/dataset/dataset_pds3_cassini_iss.py (2)
src/nav/dataset/dataset_pds3.py (2)
DataSetPDS3(18-821)add_selection_arguments(196-279)src/nav/dataset/dataset.py (11)
add_selection_arguments(108-116)pds4_bundle_template_dir(155-168)pds4_bundle_name(170-180)pds4_bundle_path_for_image(183-194)pds4_path_stub(196-207)ImageFile(15-60)pds4_image_name_to_browse_lid(209-220)pds4_image_name_to_browse_lidvid(222-233)pds4_image_name_to_data_lid(235-246)pds4_image_name_to_data_lidvid(248-259)pds4_template_variables(261-282)
src/nav/dataset/dataset_pds3_galileo_ssi.py (3)
src/nav/dataset/dataset.py (10)
ImageFile(15-60)pds4_bundle_template_dir(155-168)pds4_bundle_name(170-180)pds4_bundle_path_for_image(183-194)pds4_path_stub(196-207)pds4_image_name_to_browse_lid(209-220)pds4_image_name_to_browse_lidvid(222-233)pds4_image_name_to_data_lid(235-246)pds4_image_name_to_data_lidvid(248-259)pds4_template_variables(261-282)src/nav/dataset/dataset_pds3_voyager_iss.py (9)
pds4_bundle_template_dir(189-204)pds4_bundle_name(206-211)pds4_bundle_path_for_image(214-216)pds4_path_stub(218-220)pds4_image_name_to_browse_lid(222-231)pds4_image_name_to_browse_lidvid(233-242)pds4_image_name_to_data_lid(244-253)pds4_image_name_to_data_lidvid(255-264)pds4_template_variables(266-273)src/nav/config/config.py (1)
pds4(227-231)
🪛 Ruff (0.14.10)
src/main/nav_backplanes.py
161-163: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/nav/config/__init__.py
6-6: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
8-13: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
src/nav/config/config_helper.py
34-37: Avoid specifying long messages outside the exception class
(TRY003)
68-71: Avoid specifying long messages outside the exception class
(TRY003)
102-105: Avoid specifying long messages outside the exception class
(TRY003)
src/pds4/bundle_data.py
34-35: Avoid specifying long messages outside the exception class
(TRY003)
45-45: Use explicit conversion flag
Replace with conversion flag
(RUF010)
src/main/nav_create_bundle.py
40-40: Boolean-typed positional argument in function definition
(FBT001)
40-40: Boolean default positional argument in function definition
(FBT002)
src/backplanes/backplanes_bodies.py
54-54: Do not catch blind exception: Exception
(BLE001)
114-114: Avoid specifying long messages outside the exception class
(TRY003)
144-144: Avoid specifying long messages outside the exception class
(TRY003)
147-147: Avoid specifying long messages outside the exception class
(TRY003)
149-149: Logging statement uses f-string
(G004)
src/pds4/collections.py
36-36: Avoid specifying long messages outside the exception class
(TRY003)
src/backplanes/backplanes_rings.py
53-53: Avoid specifying long messages outside the exception class
(TRY003)
70-70: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Avoid specifying long messages outside the exception class
(TRY003)
75-75: Logging statement uses f-string
(G004)
src/nav/dataset/dataset_pds3_cassini_iss.py
443-443: Unused method argument: backplane_metadata
(ARG002)
src/backplanes/backplanes.py
40-41: Avoid specifying long messages outside the exception class
(TRY003)
48-48: Use explicit conversion flag
Replace with conversion flag
(RUF010)
64-64: Avoid specifying long messages outside the exception class
(TRY003)
69-69: Avoid specifying long messages outside the exception class
(TRY003)
71-71: Logging statement uses f-string
(G004)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test nav (ubuntu-latest, 3.10)
- GitHub Check: Test nav (ubuntu-latest, 3.11)
- GitHub Check: Test nav (ubuntu-latest, 3.12)
- GitHub Check: Test nav (ubuntu-latest, 3.13)
🔇 Additional comments (50)
src/pds4/templates/cassini_iss_saturn_1.0/bundle.lblx (1)
1-229: PDS4 bundle template structure looks good.The template follows PDS4 standards with proper Identification_Area, Context_Area, Reference_List, Bundle, and Bundle_Member_Entry sections. Template variables (
$BUNDLE_LID$,$CURRENT_ZULU(date_only=True)$,$EARLIEST_START_DATE_TIME$,$LATEST_STOP_DATE_TIME$) are appropriately placed for dynamic content generation.The contributor sequence numbers (Lines 46, 64, 82, 100) are now correctly ordered 1-4 within the List_Contributor section.
docs/developer_guide_extending.rst (1)
36-75: Documentation for PDS4 bundle methods is clear and comprehensive.The new section provides good guidance for implementing PDS4 bundle generation. The method descriptions include return types and purpose.
Minor observation: The documentation lists 9 methods but the bullet list shows 8 (combining LID and LIDVID variants). This is fine as written.
src/pds4/templates/cassini_iss_saturn_1.0/readme.txt (1)
1-44: Readme content now correctly describes backplane products.The content has been updated to properly reference Cassini ISS backplane products instead of F ring mosaics.
docs/user_guide_pds4_bundle.rst (1)
1-408: Comprehensive and well-structured PDS4 bundle generation documentation.The user guide provides thorough coverage of the bundle generation workflow, including:
- Clear explanation of the two-pass workflow (Labels and Summary)
- Complete CLI argument documentation
- Input/output file specifications
- Configuration options with priority order
- Troubleshooting guidance
src/nav/config/__init__.py (1)
3-6: Re-exports correctly added for centralized root resolution.The new helper functions are properly imported and will be available to consumers of the
nav.configmodule.Note: The static analysis hint about unused
noqadirective (RUF100) appears to be a false positive - the# noqa: F401is needed because these are re-exports that aren't directly used in this module.src/main/nav_backplane_viewer.py (2)
41-43: Import of centralized root getters is correct.The imports align with the new centralized root resolution pattern.
1442-1446: Centralized root resolution correctly implemented.The code now uses the centralized helper functions
get_nav_results_root()andget_backplane_results_root()instead of inline resolution logic. This improves consistency across the codebase.The use of
FileCache(None).new_path()is consistent with the pattern used in other modules after this refactor.src/main/nav_offset.py (3)
30-31: Import of centralized root getter is correct.The import aligns with the new centralized root resolution pattern used across the codebase.
179-180: Centralized root resolution correctly implemented.The code now uses
get_nav_results_root()for consistent root path resolution across the navigation tools. TheFileCache(None).new_path()pattern is consistent with the refactored approach in other modules.
57-57: Usage messages simplified.The usage messages now use a cleaner format without the script extension. This is a minor improvement in user-facing output.
src/main/nav_offset_cloud_tasks.py (1)
69-76: Clean integration with centralized root resolution.The use of
get_nav_results_rootwith try/except forValueErrorand structured error payloads is consistent with the pattern used across other cloud-task handlers in this PR. The refactor cleanly replaces ad-hoc config/env fallbacks.src/pds4/templates/cassini_iss_saturn_1.0/collection_context.lblx (1)
1-154: Template structure looks correct.The PDS4 Product_Collection template follows the expected structure with proper schema references, identification, citation, and inventory sections. The file metadata placeholders now consistently use the
COLLECTION_CONTEXT_CSV_PATHvariable. The hardcoded bundle identifier (rsfrench2027) is noted to be addressed in #71.src/main/nav_backplanes.py (3)
124-128: Clean refactor to centralized root resolution.The use of
get_nav_results_rootandget_backplane_results_rootwithFileCache(None).new_path()is consistent with the pattern used across the codebase. This eliminates ad-hoc config/environment fallback logic.
133-139: Good addition of startup diagnostics.The verbose logging of configuration state (dataset, roots, flags, arguments) at startup aids debugging and operational visibility.
152-164: Exception-based flow control is appropriate here.The try/except pattern for
FileNotFoundErrorprovides a clean way to skip images with missing metadata while continuing the batch. The error message includes both the image path and the exception details.src/pds4/templates/cassini_iss_saturn_1.0/collection_document.lblx (1)
1-156: Template structure is consistent with other collection templates.The PDS4 Product_Collection template for documents follows the same pattern as other collection templates. File metadata placeholders correctly use
COLLECTION_DOCUMENT_CSV_PATH. The hardcoded bundle identifier is noted to be addressed in #71.src/pds4/bundle_data.py (1)
113-118: The local file copy approach won't work for cloud storage.The TODO on line 115 correctly identifies that
shutil.copy2with local paths won't work when files are in cloud storage. The current implementation:
- Gets local paths (which may download files)
- Creates local directory
- Copies locally
- Uploads result
This may work for local testing but needs refactoring for cloud-native operation.
Please confirm this is intentional for the initial implementation and tracked for follow-up.
src/main/nav_create_bundle_cloud_tasks.py (1)
48-74: Clean integration with centralized root resolution helpers.The try/except pattern for
ValueErrorfrom root resolution helpers with structured error payloads is consistent across all cloud-task handlers. This provides clear, actionable error messages.src/backplanes/backplanes_bodies.py (4)
12-62: LGTM: Simulated backplane helper is well-structured.The helper correctly handles multiple fallback strategies for mask generation: body mask map → body index map → full rect fallback. The hash-based RNG seeding approach is acceptable per prior learnings.
113-114: Error message grammar is now correct.The message "Configuration has no bodies section for backplanes" accurately describes the error condition. This addresses the previous review comment.
140-180: Result assignment now correctly placed outside inner loop.The
result[body_name]assignment (lines 182-187) is now outside thefor bp_cfg in bodies_cfg:loop, ensuring all backplane types are accumulated before assignment. This correctly addresses the critical issue from the previous review.
172-180: Verify unit conversion for statistics is intentional.When
units.lower() == 'rad', the code convertsvalid_valuesto degrees before computing min/max. This means the statistics are stored in degrees even though the backplane arrays remain in radians. Ensure this asymmetry is intentional and documented, as consumers of the statistics must be aware the units differ from the arrays.src/main/nav_create_bundle.py (1)
242-257: LGTM: Main entry point dispatch is clean.The subcommand dispatch logic correctly validates input and routes to the appropriate handler.
src/backplanes/backplanes.py (3)
14-35: LGTM: Function signature and docstring are now consistent.The return type is
Noneand the docstring no longer claims to return a dictionary. This addresses the previous review comment.
63-65: Good: Now raisesTypeErrorfor type validation.Using
TypeErrorinstead ofValueErroris semantically correct for type mismatches.
77-98: LGTM: Backplane creation and writing flow is well-structured.The function properly chains body backplanes → ring backplanes → merge → write_fits, passing the logger and result structures consistently.
src/pds4/collections.py (5)
43-51: Good: Image name extraction is now handled by a dedicated helper.The
get_image_name_from_labelhelper properly splits on_backplanesto extract the image name, fixing the previous bug wherestem.replace('_backplanes.lblx', '')would never match.
61-64: Stem replacement is now correct.The code now uses
label_file.stem.replace('_backplanes', '')which correctly removes the suffix from the stem. This addresses the critical bug from the previous review.
99-102: Browse collection also correctly extracts image name.Same fix applied consistently for the browse collection CSV generation.
289-303: Bodies template now has proper error handling.The try/except block around
template.writelogs the error with context and re-raises, providing consistent error handling.
305-320: Rings template error handling is now consistent with bodies.The rings template write (lines 313-318) now has the same try/except pattern as the bodies template, addressing the previous review comment about inconsistent error handling.
src/backplanes/writer.py (1)
14-36: LGTM: Function signature clearly documents new parameters.The docstring accurately describes the new
bodies_resultandrings_resultparameters.src/nav/dataset/dataset.py (2)
23-32: Good:extra_paramsfield extends ImageFile for flexible parameter passing.The new
extra_paramsfield withdefault_factory=dictallows passing additional parameters to observation creation without breaking existing code.
154-282: LGTM: PDS4 interface stubs are appropriately designed.Using
NotImplementedErrorinstead of@abstractmethodis a reasonable choice since not all datasets need to support PDS4 bundle generation. The inline comments explaining this design decision are helpful.src/nav/dataset/dataset_pds3_galileo_ssi.py (2)
198-213: LGTM: Template directory resolution follows established pattern.The implementation correctly checks config first, provides a sensible default, handles absolute paths, and resolves relative paths against the templates directory. This matches the pattern used in other dataset implementations.
215-220: LGTM: Bundle name resolution is consistent.Config-driven with fallback default, matching the Voyager and New Horizons implementations.
src/nav/dataset/dataset_pds3_newhorizons_lorri.py (2)
182-197: LGTM: Template directory resolution matches established pattern.Consistent with Galileo SSI implementation.
199-204: LGTM: Bundle name resolution is consistent.Follows the same pattern as other dataset implementations.
src/nav/dataset/dataset_pds3_voyager_iss.py (3)
189-204: LGTM on template directory resolution logic.The implementation correctly checks config first, falls back to default, and handles both absolute and relative paths. This matches the pattern in
DataSetPDS3CassiniISS.
206-211: LGTM on bundle name resolution.Config-first lookup with sensible default follows the established pattern.
213-273: Stub methods are appropriate for first pass.The
NotImplementedErrorstubs for the remaining PDS4 methods are acceptable for this initial implementation, matching the base class interface fromdataset.py.docs/developer_guide_class_hierarchy.rst (4)
28-34: LGTM on class diagram updates.The PDS4 method additions to the
DataSetabstract class are correctly documented.
55-63: LGTM on new Cassini ISS subclass documentation.The
DataSetPDS3CassiniISSCruiseandDataSetPDS3CassiniISSSaturnclasses are correctly documented with their public interface.
259-260: LGTM on inheritance diagram.The inheritance relationships correctly show the new Cassini ISS subclasses extending
DataSetPDS3CassiniISS.
337-354: LGTM on narrative documentation.The text accurately describes the new PDS4 methods, dataset subclasses with their volume ranges, and updated dataset name mappings.
src/nav/dataset/dataset_pds3_cassini_iss.py (5)
311-333: LGTM on template directory resolution.The implementation correctly checks config, falls back to subclass default, and handles absolute vs relative paths.
335-346: LGTM on bundle name resolution.Config-first lookup with subclass-provided default follows the established pattern.
456-598: Comprehensive template variable mapping.The
pds4_template_variablesmethod provides thorough mapping from PDS3 index columns to PDS4 template variables. The hardcoded placeholder values for compression parameters (lines 550-553) and theSOURCE_IMAGE_LIDVIDplaceholder (lines 493-495) are acceptable for a first pass but should be tracked for resolution before production use.
600-622: Reasonable use of NotImplementedError for abstract-like methods.Raising
NotImplementedErrorin the baseDataSetPDS3CassiniISSclass for_dataset_name_for_pds4_config,_default_pds4_template_dir, and_default_pds4_bundle_nameensures subclasses must provide their own implementations. This is appropriate since the base class includes all volumes and PDS4 bundle generation requires mission-phase-specific configuration.
625-656: LGTM on new Cassini ISS subclasses.
DataSetPDS3CassiniISSCruiseandDataSetPDS3CassiniISSSaturncorrectly:
- Override
_ALL_VOLUME_NAMESwith appropriate volume ranges- Implement required PDS4 configuration methods with mission-phase-specific values
- Follow naming conventions consistent with the dataset mapping in
__init__.py
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/main/nav_create_bundle_cloud_tasks.py (1)
118-127: Add exception handling for missing metadata files.
generate_bundle_data_filescan raiseFileNotFoundErrorwhen navigation or backplane metadata files are missing. Without a try/except, the task will crash rather than returning a structured error.🔎 Proposed fix
+ try: generate_bundle_data_files( dataset=dataset, image_files=ImageFiles(image_files=image_files), nav_results_root=nav_results_root, backplane_results_root=backplane_results_root, bundle_results_root=bundle_results_root, logger=DEFAULT_LOGGER ) + except FileNotFoundError as e: + return False, { + 'status': 'error', + 'status_error': 'missing_metadata', + 'status_exception': str(e) + } return False, {'status': 'success'} # No retry under any circumstancessrc/main/nav_create_bundle.py (2)
164-181: Add exception handling to continue processing on individual image failures.The call to
generate_bundle_data_filesis unprotected. If processing one image fails (e.g., missing metadata), the entire run stops. Consider wrapping in a try/except to log the error and continue with remaining images.🔎 Proposed fix
if arguments.dry_run: MAIN_LOGGER.info('Would process: %s', imagefiles.image_files[0].label_file_url.as_posix()) continue + try: generate_bundle_data_files( dataset=DATASET, image_files=imagefiles, nav_results_root=nav_results_root, backplane_results_root=backplane_results_root, bundle_results_root=bundle_results_root, logger=MAIN_LOGGER ) + except Exception: + MAIN_LOGGER.exception('Failed to process: %s', + imagefiles.image_files[0].label_file_url.as_posix()) + continue
189-198: Config loading duplicates pattern from main_labels (optional refactor).Lines 189-198 manually load configuration using the same pattern as the helper
load_default_and_user_configused inmain_labels(line 145). Consider using the helper here as well for consistency:- # Read configuration files - DEFAULT_CONFIG.read_config() - if arguments.config_file: - for config_file in arguments.config_file: - DEFAULT_CONFIG.update_config(config_file) - else: - try: - DEFAULT_CONFIG.update_config('nav_default_config.yaml') - except FileNotFoundError: - pass + load_default_and_user_config(arguments, DEFAULT_CONFIG)src/main/nav_backplanes_cloud_tasks.py (1)
109-117: Add exception handling and return consistent success payload.Two issues:
Missing exception handling:
generate_backplanes_image_filescan raiseFileNotFoundErrorwhen metadata is missing. Unlikenav_backplanes.py(lines 145-157) which catches this, the cloud-tasks version will crash the task.Inconsistent success response: Returns
(False, None)on success whilenav_create_bundle_cloud_tasks.pyreturns(False, {'status': 'success'}).🔎 Proposed fix
+ try: generate_backplanes_image_files( obs_class, ImageFiles(image_files=image_files), nav_results_root=nav_results_root, backplane_results_root=backplane_results_root, write_output_files=True ) + except FileNotFoundError as e: + return False, { + 'status': 'error', + 'status_error': 'missing_metadata', + 'status_exception': str(e) + } - return False, None # No retry under any circumstances + return False, {'status': 'success'} # No retry under any circumstances
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (17)
docs/user_guide_backplanes.rstdocs/user_guide_pds4_bundle.rstsrc/backplanes/writer.pysrc/main/nav_backplanes.pysrc/main/nav_backplanes_cloud_tasks.pysrc/main/nav_create_bundle.pysrc/main/nav_create_bundle_cloud_tasks.pysrc/main/nav_offset.pysrc/main/nav_offset_cloud_tasks.pysrc/nav/config/__init__.pysrc/nav/config/config_helper.pysrc/nav/dataset/dataset_pds3.pysrc/nav/dataset/dataset_pds3_galileo_ssi.pysrc/nav/dataset/dataset_pds3_newhorizons_lorri.pysrc/nav/dataset/dataset_pds3_voyager_iss.pysrc/pds4/collections.pysrc/pds4/templates/cassini_iss_saturn_1.0/readme.txt
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-11-24T19:48:51.676Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 48
File: main/nav_main_offset.py:167-171
Timestamp: 2025-11-24T19:48:51.676Z
Learning: In the nav_main_offset.py file, it is normal and expected for the nav_default_config.yaml file to be missing. The FileNotFoundError suppression when attempting to load this default config is intentional behavior, not an error condition that needs logging.
Applied to files:
src/main/nav_offset_cloud_tasks.pysrc/main/nav_offset.pysrc/main/nav_create_bundle_cloud_tasks.pysrc/main/nav_backplanes.pysrc/main/nav_backplanes_cloud_tasks.py
📚 Learning: 2025-12-23T19:46:29.815Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 68
File: src/nav/dataset/dataset_pds3_cassini_iss.py:241-241
Timestamp: 2025-12-23T19:46:29.815Z
Learning: In Python, for staticmethod methods, super() cannot be used because there is no class or instance context. If a subclass static method needs to call a parent class's static method, invoke it directly as ParentClass.method_name() rather than super().method_name(). This applies broadly to Python files in the repository; ensure the parent method is defined as a static method or accessible in the class hierarchy and that imports are correct.
Applied to files:
src/main/nav_offset_cloud_tasks.pysrc/main/nav_offset.pysrc/nav/config/config_helper.pysrc/backplanes/writer.pysrc/main/nav_create_bundle_cloud_tasks.pysrc/main/nav_backplanes.pysrc/main/nav_create_bundle.pysrc/nav/dataset/dataset_pds3_voyager_iss.pysrc/main/nav_backplanes_cloud_tasks.pysrc/nav/dataset/dataset_pds3.pysrc/nav/config/__init__.pysrc/pds4/collections.pysrc/nav/dataset/dataset_pds3_newhorizons_lorri.pysrc/nav/dataset/dataset_pds3_galileo_ssi.py
📚 Learning: 2025-09-22T21:03:10.431Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 1
File: tests/nav/dataset/test_dataset_pds3_newhorizons_lorri.py:27-33
Timestamp: 2025-09-22T21:03:10.431Z
Learning: PDS3 (Planetary Data System version 3) archives are immutable and never updated once finalized. Exact count assertions in tests for PDS3 datasets are appropriate since the underlying data will never change.
Applied to files:
src/nav/dataset/dataset_pds3.pysrc/nav/dataset/dataset_pds3_newhorizons_lorri.pysrc/nav/dataset/dataset_pds3_galileo_ssi.py
📚 Learning: 2025-09-22T23:18:15.371Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 1
File: tests/nav/dataset/test_dataset_pds3_cassini_iss.py:29-34
Timestamp: 2025-09-22T23:18:15.371Z
Learning: PDS3 dataset indices are stable and immutable once archived, making exact count assertions appropriate in tests rather than using lower-bound checks.
Applied to files:
src/nav/dataset/dataset_pds3.py
📚 Learning: 2025-11-10T22:24:08.880Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 14
File: nav/obs/__init__.py:36-44
Timestamp: 2025-11-10T22:24:08.880Z
Learning: In nav/obs/__init__.py, the __all__ list should maintain semantic grouping rather than alphabetical ordering: generic classes (Obs, ObsSnapshot, ObsSnapshotInst) first, followed by specific instrument classes (ObsCassiniISS, etc.), then helper functions (inst_names, inst_name_to_obs_class).
Applied to files:
src/nav/config/__init__.py
📚 Learning: 2025-12-23T19:48:24.234Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 68
File: src/pds4/collections.py:293-303
Timestamp: 2025-12-23T19:48:24.234Z
Learning: In the rms-nav repository, pdstemplate.write() automatically handles uploading to remote storage when the target path is an FCPath or URL-based path. Explicit .upload() calls after pdstemplate.write() are redundant for FCPath objects.
Applied to files:
src/pds4/collections.py
📚 Learning: 2025-12-04T23:41:21.874Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 52
File: nav/obs/obs_inst_voyager_iss.py:47-49
Timestamp: 2025-12-04T23:41:21.874Z
Learning: In the rms-nav codebase, `FCPath.get_local_path()` from the filecache library returns `Path | list[Path]`. When used with a single path argument, `cast(Path, fc_path.get_local_path())` is required to narrow the type to `Path` for type checking purposes. This pattern is intentionally used across observation instrument files (obs_inst_voyager_iss.py, obs_inst_newhorizons_lorri.py, obs_inst_cassini_iss.py, obs_inst_galileo_ssi.py).
Applied to files:
src/nav/dataset/dataset_pds3_galileo_ssi.py
🧬 Code graph analysis (10)
src/main/nav_offset_cloud_tasks.py (3)
src/nav/config/config_helper.py (2)
get_nav_results_root(45-79)load_default_and_user_config(120-141)src/nav/dataset/__init__.py (1)
dataset_name_to_inst_name(72-84)src/nav/obs/__init__.py (1)
inst_name_to_obs_class(26-35)
src/main/nav_offset.py (2)
src/nav/config/config_helper.py (2)
get_nav_results_root(45-79)load_default_and_user_config(120-141)src/nav/dataset/__init__.py (2)
dataset_names(52-54)dataset_name_to_class(57-69)
src/nav/config/config_helper.py (1)
src/nav/config/config.py (4)
Config(9-231)environment(119-123)read_config(60-81)update_config(83-102)
src/backplanes/writer.py (3)
src/nav/support/file.py (1)
json_as_string(94-102)src/nav/obs/obs_snapshot.py (1)
closest_planet(302-303)src/nav/config/config.py (2)
Config(9-231)satellites(132-145)
src/main/nav_create_bundle_cloud_tasks.py (3)
src/nav/dataset/__init__.py (1)
dataset_name_to_class(57-69)src/nav/config/config_helper.py (4)
get_backplane_results_root(7-42)get_nav_results_root(45-79)get_pds4_bundle_results_root(82-117)load_default_and_user_config(120-141)src/pds4/bundle_data.py (1)
generate_bundle_data_files(14-128)
src/main/nav_backplanes.py (3)
src/nav/config/config_helper.py (3)
get_backplane_results_root(7-42)get_nav_results_root(45-79)load_default_and_user_config(120-141)src/nav/dataset/__init__.py (2)
dataset_names(52-54)dataset_name_to_class(57-69)src/backplanes/backplanes.py (1)
generate_backplanes_image_files(18-99)
src/main/nav_create_bundle.py (3)
src/nav/dataset/__init__.py (2)
dataset_names(52-54)dataset_name_to_class(57-69)src/pds4/bundle_data.py (1)
generate_bundle_data_files(14-128)src/pds4/collections.py (2)
generate_collection_files(13-125)generate_global_index_files(128-322)
src/main/nav_backplanes_cloud_tasks.py (2)
src/nav/config/config_helper.py (3)
get_backplane_results_root(7-42)get_nav_results_root(45-79)load_default_and_user_config(120-141)src/backplanes/backplanes.py (1)
generate_backplanes_image_files(18-99)
src/nav/config/__init__.py (1)
src/nav/config/config_helper.py (4)
get_backplane_results_root(7-42)get_nav_results_root(45-79)get_pds4_bundle_results_root(82-117)load_default_and_user_config(120-141)
src/nav/dataset/dataset_pds3_galileo_ssi.py (4)
src/nav/dataset/dataset.py (10)
ImageFile(15-60)pds4_bundle_template_dir(155-168)pds4_bundle_name(170-180)pds4_bundle_path_for_image(183-194)pds4_path_stub(196-207)pds4_image_name_to_browse_lid(209-220)pds4_image_name_to_browse_lidvid(222-233)pds4_image_name_to_data_lid(235-246)pds4_image_name_to_data_lidvid(248-259)pds4_template_variables(261-282)src/nav/dataset/dataset_pds3_newhorizons_lorri.py (9)
pds4_bundle_template_dir(182-197)pds4_bundle_name(199-204)pds4_bundle_path_for_image(207-209)pds4_path_stub(211-213)pds4_image_name_to_browse_lid(215-224)pds4_image_name_to_browse_lidvid(226-235)pds4_image_name_to_data_lid(237-246)pds4_image_name_to_data_lidvid(248-257)pds4_template_variables(259-267)src/nav/dataset/dataset_pds3_voyager_iss.py (9)
pds4_bundle_template_dir(189-204)pds4_bundle_name(206-211)pds4_bundle_path_for_image(214-216)pds4_path_stub(218-220)pds4_image_name_to_browse_lid(222-231)pds4_image_name_to_browse_lidvid(233-242)pds4_image_name_to_data_lid(244-253)pds4_image_name_to_data_lidvid(255-264)pds4_template_variables(266-274)src/nav/config/config.py (1)
pds4(227-231)
🪛 Ruff (0.14.10)
src/nav/config/config_helper.py
38-41: Avoid specifying long messages outside the exception class
(TRY003)
75-78: Avoid specifying long messages outside the exception class
(TRY003)
113-116: Avoid specifying long messages outside the exception class
(TRY003)
134-134: Consider moving this statement to an else block
(TRY300)
src/main/nav_backplanes.py
154-156: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/nav/config/__init__.py
7-7: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
9-15: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
src/pds4/collections.py
36-36: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test nav (ubuntu-latest, 3.12)
- GitHub Check: Test nav (ubuntu-latest, 3.13)
- GitHub Check: Test nav (ubuntu-latest, 3.11)
- GitHub Check: Test nav (ubuntu-latest, 3.10)
🔇 Additional comments (43)
src/nav/dataset/dataset_pds3_galileo_ssi.py (3)
3-6: LGTM! Import addition is necessary.The
ImageFileimport is required for the type annotation inpds4_template_variablesmethod signature.
198-220: LGTM! Implementations follow established patterns.The
pds4_bundle_template_dirandpds4_bundle_namemethods correctly follow the same configuration-driven pattern used in Voyager ISS and New Horizons LORRI dataset classes, with appropriate defaults for Galileo SSI.
222-283: LGTM! Stub methods are consistent with other dataset implementations.All PDS4-related stub methods follow the same signatures and patterns as Voyager ISS and New Horizons LORRI datasets. The
pds4_template_variablesmethod now correctly includes the keyword-only marker (*,) at line 277, resolving the previously flagged signature mismatch. TheNotImplementedErrorplaceholders are appropriate for this initial PDS4 implementation pass.src/nav/dataset/dataset_pds3_newhorizons_lorri.py (9)
6-6: LGTM: Import added for PDS4 extension methods.The ImageFile import is necessary for type hints in the new PDS4-related methods.
182-197: LGTM: Configuration-driven template directory resolution.The method correctly resolves template directories from configuration with appropriate fallback to a default template name, and handles both absolute and relative paths.
199-204: LGTM: Simple configuration-driven bundle name getter.The method provides appropriate configuration lookup with a reasonable default value.
211-213: Intentional extension point established.This placeholder method is part of the PDS4 scaffolding pattern for path stub generation.
215-224: Intentional extension point established.This placeholder method is part of the PDS4 scaffolding pattern for LID mapping. The docstring correctly describes the method's purpose.
226-235: Intentional extension point established with correct documentation.This placeholder method is part of the PDS4 scaffolding pattern. The docstring correctly describes returning a LIDVID (the previous docstring inconsistency has been resolved).
237-246: Intentional extension point established.This placeholder method is part of the PDS4 scaffolding pattern for data LID mapping. The docstring correctly describes the method's purpose.
248-257: Intentional extension point established.This placeholder method is part of the PDS4 scaffolding pattern for data LIDVID mapping. The docstring correctly describes the method's purpose.
259-267: Intentional extension point established with correct signature.This placeholder method is part of the PDS4 scaffolding pattern for template variable generation. The signature correctly uses keyword-only parameters (the previous signature mismatch has been resolved).
src/nav/config/__init__.py (2)
3-7: LGTM - Imports are clean and well-organized.The new imports from
config_helperexpose essential path resolution and configuration loading utilities that are consumed by multiple CLI modules (backplanes, offset, bundle generation).
9-15:__all__ordering is semantically grouped and appropriate.The current ordering (Config objects first, then helper functions) is consistent with the repository's preference for semantic grouping over strict alphabetical sorting. Static analysis suggesting alphabetical sorting can be safely ignored here.
Based on learnings, semantic grouping is preferred for
__all__lists in this repository.src/nav/dataset/dataset_pds3_voyager_iss.py (2)
189-211: LGTM - Config-driven PDS4 bundle defaults are well-implemented.The template directory and bundle name methods properly fall back to sensible defaults (
voyager_iss_1.0andvoyager_iss_backplanes_rsfrench2027) when not configured, and correctly resolve relative paths relative to the pds4/templates directory.
213-274: LGTM - PDS4 method placeholders provide clear extension points.These NotImplementedError placeholders establish the PDS4 interface for future implementation while documenting the expected signatures and return types. The docstrings correctly distinguish between LID and LIDVID methods.
src/backplanes/writer.py (2)
14-36: LGTM - Function signature refactored to support new metadata workflow.The function signature changes (removing
label_file_path, addingbodies_resultandrings_result) align with the broader PR refactoring that moves PDS4 label generation to a separate bundle generation pass. The new parameters enable metadata aggregation from backplane creation results.
71-124: LGTM - Metadata construction and JSON serialization are well-structured.The backplane metadata construction properly:
- Initializes
invto an empty dict (line 89) when closest_planet is None, avoiding UnboundLocalError- Extracts per-body statistics and inventory data (lines 92-117)
- Correctly reorders center_uv from [u, v] to [v, u] format (lines 103-104)
- Handles optional ring statistics (lines 119-121)
- Writes clean JSON via
json_as_stringhelper (line 123)docs/user_guide_backplanes.rst (1)
27-28: LGTM - Documentation updates are clear and concise.The updated descriptions for
--nav-results-rootand--backplane-results-rootaccurately reflect the centralized root resolution behavior introduced in the config helpers.src/pds4/templates/cassini_iss_saturn_1.0/readme.txt (1)
1-44: LGTM - Bundle readme content is accurate and well-structured.The readme correctly describes the Cassini ISS Saturn backplanes bundle with:
- Appropriate content description (backplanes products, not F-ring mosaics)
- Consistent URN identifiers throughout
- Clear citation template with TODO for DOI
- Contact information for support
Previous concerns about F-ring content mismatch have been addressed.
src/main/nav_offset.py (2)
30-32: LGTM - Config refactoring centralizes root resolution cleanly.The refactoring consolidates configuration loading and navigation root resolution into shared helper functions from
nav.config, eliminating duplicated logic. The use ofFileCache(None).new_path()is consistent with the pattern indataset_pds3.py(line 59).Also applies to: 167-171
58-58: LGTM - Usage string updates make CLI documentation more generic.Removing the script name prefix (
python nav_main_offset.py) from usage messages makes the documentation cleaner and more suitable for installed CLI commands.Also applies to: 66-66, 74-74
docs/user_guide_pds4_bundle.rst (2)
1-400: LGTM - Comprehensive PDS4 bundle generation documentation.The documentation provides clear, thorough coverage of:
- Two-pass workflow (labels and summary passes)
- Bundle directory structure and file organization
- CLI interfaces for both local and cloud-based processing
- Configuration options and template system
- Complete workflow examples
- Troubleshooting guidance
This is excellent documentation for users implementing PDS4 bundle generation.
93-95: No issues found. The CLI argument--bundle-results-rootcorrectly converts toarguments.bundle_results_rootin the argparse Namespace, and the implementation inconfig_helper.pycorrectly accesses this attribute.src/main/nav_offset_cloud_tasks.py (2)
25-67: LGTM! Solid refactoring to centralized configuration helpers.The migration to
load_default_and_user_configandget_nav_results_rooteliminates ad-hoc configuration handling and standardizes error responses across cloud-tasks entry points.
72-115: LGTM! Consistent error handling across validation paths.All validation failures now return structured error payloads with
statusandstatus_errorkeys, enabling better monitoring and retry logic in the cloud-tasks framework.src/main/nav_backplanes.py (3)
26-29: LGTM! Configuration loading aligns with centralized pattern.The migration to
load_default_and_user_configand the new root resolution helpers (get_nav_results_root,get_backplane_results_root) eliminates environment-variable fallback code and centralizes the resolution logic.Also applies to: 114-121
126-132: LGTM! Enhanced startup logging improves observability.Logging the configuration state (roots, flags, arguments) at startup aids in debugging and operational monitoring.
145-157: LGTM! Exception handling allows continued processing despite missing metadata.The try/except block gracefully handles missing metadata files, logs the error with context, and continues processing remaining images.
src/main/nav_backplanes_cloud_tasks.py (2)
24-57: LGTM! Centralized root resolution with consistent error handling.Both navigation and backplane roots are resolved using the new helpers with proper try/except blocks returning structured error payloads.
60-100: LGTM! Validation errors use consistent structured payloads.All validation failures return dictionaries with
statusandstatus_errorkeys, improving monitoring and debugging capabilities.src/main/nav_create_bundle_cloud_tasks.py (2)
24-66: LGTM! Consistent root resolution across all three required roots.Navigation, backplane, and bundle roots are all resolved using the centralized helpers with proper error handling.
68-116: LGTM! Comprehensive validation with consistent error reporting.All validation steps return structured error payloads, enabling proper error tracking and monitoring.
src/nav/config/config_helper.py (1)
120-141: LGTM! Config loading centralizes the pattern used across entry points.The function properly handles user-specified config files with fallback to the default. Based on learnings, the
FileNotFoundErrorsuppression fornav_default_config.yamlis intentional.src/pds4/collections.py (5)
13-52: LGTM! Clear structure for collection file generation.The function properly scans for label files, validates directory existence, and sorts by image name.
54-90: LGTM! Collection data generation with proper error handling.The CSV and label generation follow proper patterns with exception handling around template writes.
92-125: LGTM! Browse collection generation mirrors data collection pattern.Consistent structure with proper error handling for template operations.
128-226: LGTM! Global index row collection with resilient error handling.The function scans supplemental files and builds index rows for bodies and rings. The exception handling at lines 178-181 allows processing to continue when individual files fail, as acknowledged in past reviews.
228-322: LGTM! Index file and label generation with consistent error handling.Both bodies and rings index files are generated with proper formatting, and labels are rendered with matching exception handling. As noted in past reviews,
pdstemplate.write()handles upload automatically for FCPath objects.src/main/nav_create_bundle.py (4)
40-72: LGTM! Common argument helper with proper keyword-only parameter.The
for_labelsparameter is now keyword-only, improving call-site clarity as suggested in past reviews.
74-136: LGTM! Argument parsers with proper dataset validation.Both subcommand parsers validate dataset names early with clear error messages.
139-162: LGTM! Config loading and root resolution use centralized helpers.The labels subcommand properly loads configuration and resolves all required roots using the new helper functions.
211-234: LGTM! Summary generation with proper error handling and cleanup.Both collection and global index generation are wrapped in try/except blocks with appropriate logging and exit behavior.
This is the initial pass at PDS4 bundle generation.
nav_create_bundleandnav_create_bundle_cloud_tasks.pds4section to config.pds4with modules to create data and browse product labels and collections and global indexes.Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.