Enable virtual dataset cataloging#25
Conversation
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds dataset registration, deregistration, replace handling, Arrow-table scan helpers, remote TIFF/Zarr support, OME-Arrow wrappers, profile-table publishing, and matching docs, config, and test coverage. ChangesIngestion and publishing workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/iceberg_bioimage/api.py (1)
32-33: ⚡ Quick winAvoid relying on private
_dataset_idacross modules.At Line 32, importing
_dataset_idfrom another module couples this API to a private implementation detail. Please promote a public helper (or move it to a shared utility module) to avoid brittle cross-file contracts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/iceberg_bioimage/api.py` around lines 32 - 33, The import of the private symbol `_dataset_id` into this module creates a fragile cross-module dependency; refactor by exposing a public helper (e.g., rename `_dataset_id` to `get_dataset_id` or `dataset_id_for` and place it in a shared utility module) and update callers (including `delete_dataset_image_assets`) to import and call the new public helper instead of referencing the private name; ensure any tests and all references to `_dataset_id` are updated to use the new public function and adjust docstrings/comments accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/iceberg_bioimage/api.py`:
- Around line 81-88: Currently the code calls
_deregister_by_dataset_id(_dataset_id(scan_result.source_uri), ...) when
replace=True before publishing, which can permanently drop dataset rows if
publish_image_assets(...) or publish_chunk_index(...) fails; change the flow so
deletions occur only after both publish_image_assets and publish_chunk_index
complete successfully (or implement an atomic swap/transactional approach),
i.e., defer the call to _deregister_by_dataset_id until after successful returns
from publish_image_assets and publish_chunk_index (or add rollback logic in the
exception path) and ensure _dataset_id(scan_result.source_uri) is the same id
used for the deferred deletion.
In `@src/iceberg_bioimage/publishing/image_assets.py`:
- Around line 76-83: In _build_profile_schema, replace the private import
pyiceberg.io.pyarrow._pyarrow_to_schema_without_ids with the public
pyiceberg.io.pyarrow.pyarrow_to_schema and call
pyarrow_to_schema(arrow_table.schema); if that raises or indicates missing
Iceberg field IDs, pass a generated minimal name_mapping (e.g., map each field
name to a unique id) to pyarrow_to_schema so it can assign IDs—wrap the import
and call in the same try/except and raise the same RuntimeError on ImportError,
ensuring the function returns the result of pyarrow_to_schema instead of the
private helper.
In `@tests/test_iris_datasets.py`:
- Around line 208-209: The test currently asserts that any .parquet exists in
the iris_nf1 directory (parquet_files = list(iris_nf1.glob("*.parquet")) ;
assert parquet_files...), which is too loose; update the assertion to
specifically check for the presence of "profiles.parquet" in the iris_nf1
directory (e.g., verify (iris_nf1 / "profiles.parquet").exists() or assert
"profiles.parquet" is in the list of file names from
iris_nf1.glob("*.parquet")), so the test explicitly ensures profiles.parquet is
present.
- Around line 166-171: The test currently allows vacuous success when there are
no NF1 images; before calling scan_as_arrow_table and before computing dataset
IDs ensure the image list is non-empty by asserting the tiff_files variable is
truthy. Specifically, add an assertion like "assert tiff_files" before the list
comprehension that builds tables (used with scan_as_arrow_table) and likewise
before the block that computes dataset IDs/uniqueness so the tests fail if no
.tif files are found.
In `@tests/test_profile_namespace.py`:
- Around line 212-220: The test
test_iris_nf1_profiles_alias_resolves_well_and_site currently asserts
resolved["plate_id"] but should verify site resolution; update the assertion for
the resolved mapping returned by resolve_microscopy_profile_columns to assert
resolved["site_id"] is not None (or add that assertion alongside the existing
plate_id check) so the test actually validates the "site" alias (refer to
variables well_id, plate_id, site_id and the function
resolve_microscopy_profile_columns).
---
Nitpick comments:
In `@src/iceberg_bioimage/api.py`:
- Around line 32-33: The import of the private symbol `_dataset_id` into this
module creates a fragile cross-module dependency; refactor by exposing a public
helper (e.g., rename `_dataset_id` to `get_dataset_id` or `dataset_id_for` and
place it in a shared utility module) and update callers (including
`delete_dataset_image_assets`) to import and call the new public helper instead
of referencing the private name; ensure any tests and all references to
`_dataset_id` are updated to use the new public function and adjust
docstrings/comments accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a9f2ba9-4a8c-4b0b-979b-b807e41c827b
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.pre-commit-config.yamlREADME.mddocs/src/examples/basic-workflow.pydocs/src/examples/metadata-workflow.pypyproject.tomlsrc/iceberg_bioimage/__init__.pysrc/iceberg_bioimage/adapters/ome_tiff.pysrc/iceberg_bioimage/adapters/zarr_v2.pysrc/iceberg_bioimage/api.pysrc/iceberg_bioimage/integrations/ome_arrow.pysrc/iceberg_bioimage/publishing/chunk_index.pysrc/iceberg_bioimage/publishing/image_assets.pysrc/iceberg_bioimage/validation/contracts.pytests/__init__.pytests/conftest.pytests/fakes.pytests/test_iris_datasets.pytests/test_ome_arrow_integration.pytests/test_profile_namespace.pytests/test_registration_update.pytests/test_s3_support.pytests/test_virtual_datasets.py
gwaybio
left a comment
gwaybio
left a comment
There was a problem hiding this comment.
Exciting to see this! I made several comments which should be addressed prior to merging. You'll notice the comments are concentrated in the readme, which is currently quite terse. I would recommend paying very careful attention to language and clarity, especially at this stage in the project. Paying off this debt early will be a big win long-term!
Co-Authored-By: Gregory Way <gregory.way@gmail.com>
Co-authored-by: Gregory Way <gregory.way@gmail.com>
|
Thanks @gwaybio ! After working through your comments and making changes I think this is ready for another review when there's a chance. |
There was a problem hiding this comment.
A few more comments - looking good.
The README still needs a bit of work. It's still quite terse and I'm often wondering why would I want to do this or cool, but why should I care. The README is a very important document and we need to make sure it's accurately and understandably describing the motivation for the work. It needs to self-justify, at this moment.
More README improvements, might be beyond scope of this PR, so feel free to punt for now
| Requires ``bioio`` and an OME-Zarr reader backend in the environment. | ||
| Any keyword arguments are forwarded to ``ome_arrow.from_ome_zarr``. | ||
|
|
||
| Conversion cost: ``ome_arrow.from_ome_zarr`` eagerly reads every plane of |
There was a problem hiding this comment.
this reads as more severe than your comment below - consider revising
| "plate_id": ( | ||
| "Metadata_Plate", | ||
| "Image_Metadata_Plate", | ||
| "Image_Metadata_Plate_x", |
There was a problem hiding this comment.
not sure what this file is doing, but these ("_x", "_y") seem like pandas merge artifacts rather than real metadata
| def scan_as_arrow_table(uri: str) -> pa.Table: | ||
| """Scan a supported image store and return metadata as an Arrow table. | ||
|
|
||
| No catalog or Iceberg tables are required. Drop an ome-zarr or ome-tiff |
There was a problem hiding this comment.
| No catalog or Iceberg tables are required. Drop an ome-zarr or ome-tiff | |
| No catalog or Iceberg tables are required. Drop an ome-zarr or ome-tiff |
| "Image_Metadata_Well_x", | ||
| "Image_Metadata_Plate_x", | ||
| "Image_Metadata_Site_x", |
There was a problem hiding this comment.
same comment here about potential merge artifacts
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # ome-iris NF1 integration: profiles.parquet in the catalog |
There was a problem hiding this comment.
we're testing they exist, but is there a test to ensure they are accurate?
| Parquet-backed warehouse root that tools like `pycytominer` can consume | ||
| directly. | ||
| Export images and profiles into a Parquet warehouse layout that `pycytominer` | ||
| and CytoTable can consume directly, without going through Iceberg at all. |
There was a problem hiding this comment.
why would someone choose to do this?
| catalog and join them to analysis outputs directly: | ||
| ## OME-Arrow integration | ||
|
|
||
| Everything above deals with *metadata* — shape, dtype, file location. If you |
There was a problem hiding this comment.
i like how you're describing the organization of the README here, but it is buried all the way on line 268. Consider being very explicit about the organization up front
| ``` | ||
|
|
||
| from iceberg_bioimage import join_catalog_image_assets_with_profiles | ||
| > **Conversion cost:** `create_ome_arrow_from_zarr`/`create_ome_arrow_from_tiff` |
There was a problem hiding this comment.
this describes that there is a conversion cost, but doesn't describe how much cost
| ## DuckDB helpers | ||
|
|
||
| Optional SQL-style filtering and joins over registered metadata tables, | ||
| useful when you want ad-hoc queries (e.g. "all images with `cell_count > 10`") |
| ```python | ||
| from iceberg_bioimage import join_image_assets_with_profiles, query_metadata_table | ||
|
|
||
| joined = join_image_assets_with_profiles(image_assets_table, profiles_table) |
There was a problem hiding this comment.
but I don't see here what these variables are. Also, what is "joined"?
2 participants
Description
This PR enables virtual dataset references. We also add OME-IRIS datasets for testing and upgrade the OME-Arrow requirements of the project.
What kind of change(s) are included?
Checklist
Please ensure that all boxes are checked before indicating that this pull request is ready for review.
Summary by CodeRabbit
register_store/directory registration now support replace mode._x/_yvariants.