Feature/implement tile masks#4
Merged
Merged
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a tile-mask preprocessing pipeline: new dataset/experiment/preprocessing configs, a Hydra entrypoint that downloads slide/tile parquet artifacts from MLflow, renders per-slide percentage and outline masks to BigTIFFs, and a submission script to run the pipeline on cluster jobs. Changes
Sequence Diagram(s)sequenceDiagram
participant Hydra as Hydra (entrypoint)
participant MLflow as MLflow (Artifacts)
participant Worker as Worker (process_slide)
participant PyVIPS as PyVIPS (Image IO)
participant Temp as Temp Storage (Local tmp)
Hydra->>MLflow: download `slides.parquet`, `tiles.parquet` from tiling_run_id
MLflow-->>Hydra: return parquet data
Hydra->>Hydra: group tiles by `slide_id`, build items list
Hydra->>Worker: dispatch bounded parallel jobs for each slide
par per-slide processing
Worker->>Worker: select tile_percentage_cols, compute masks (numpy)
Worker->>PyVIPS: convert numpy -> single-band image
PyVIPS->>Temp: write `<wsi_stem>.tiff` (BigTIFF) with mpp metadata
Temp-->>Worker: file created
end
Worker-->>Hydra: job complete
Hydra->>MLflow: log generated TIFF artifacts under `tile_masks`
MLflow-->>Hydra: artifacts logged
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new preprocessing script and configuration for generating tile masks using Ray and PyVips. Several critical issues were identified: the use of a local temporary directory will fail in multi-node Ray environments, the tile outline drawing logic contains potential out-of-bounds errors for small tiles, and the use of mask.tobytes() may lead to memory exhaustion. Furthermore, the default downsampling factor in the configuration is inconsistent with the intended memory budget, and the job submission script contains placeholders that must be replaced to avoid runtime failures.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
preprocessing/tile_masks.py (3)
138-141: Wrapray.init()/ray.shutdown()intry/finally.If
main()raises (Hydra config error, MLflow download failure, a worker exception bubbling up fromprocess_items, etc.),ray.shutdown()is skipped and the Ray runtime leaks for the lifetime of the process.♻️ Proposed fix
if __name__ == "__main__": - ray.init() - main() - ray.shutdown() + ray.init() + try: + main() + finally: + ray.shutdown()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@preprocessing/tile_masks.py` around lines 138 - 141, The current script calls ray.init(), main(), and ray.shutdown() sequentially, which can skip ray.shutdown() if main() raises; modify the module-level block so ray.init() is called before a try and ray.shutdown() is invoked in a finally block to guarantee cleanup even on exceptions — specifically wrap the calls around main() (the main function symbol) so that ray.shutdown() always runs regardless of errors from main() or any worker functions like process_items.
109-120: Empty fallback DataFrame defaults toobjectdtype.
pd.DataFrame(columns=["x", "y"])produces object-dtype columns. Operations in_draw_tile_outlines(xs[:, None] + np.arange(tw),>=comparisons, fancy indexing) then run on object arrays — they happen to be no-ops here only because the arrays are empty. This is brittle: any future change that hits the loop with non-empty object data will silently produce wrong indices.♻️ Pin the dtype, or skip empty slides
- items = [ - ( - slide.to_dict(), - tiles_by_slide.get(slide["id"], pd.DataFrame(columns=["x", "y"])), - ) - for _, slide in slides.iterrows() - ] + empty_tiles = pd.DataFrame({"x": pd.Series(dtype="int64"), "y": pd.Series(dtype="int64")}) + items = [ + (slide.to_dict(), tiles_by_slide.get(slide["id"], empty_tiles)) + for _, slide in slides.iterrows() + ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@preprocessing/tile_masks.py` around lines 109 - 120, The fallback empty DataFrame used when building items uses pd.DataFrame(columns=["x","y"]) which creates object dtypes and can break numeric operations in _draw_tile_outlines; change the fallback to an empty DataFrame with numeric dtypes (e.g. pd.DataFrame({"x": pd.Series(dtype=tiles["x"].dtype), "y": pd.Series(dtype=tiles["y"].dtype)}) or explicit np.int64/np.float64) or alternatively filter out slides with no tiles before constructing items so _draw_tile_outlines never receives object-typed arrays; update the code that builds tiles_by_slide/items to use this typed empty DataFrame (referencing tiles_by_slide, items and _draw_tile_outlines).
79-86: Usemask.datainstead ofmask.tobytes()to avoid unnecessary memory copying.
tobytes()creates a full copy of the array data. For a 1.5 GB downsampled mask, this momentarily requires ~3 GB resident memory before pyvips finishes consuming it — right at the Ray task'smemory=3 * 1024**3budget ceiling. numpy arrays expose their buffer directly via the.dataattribute (a memoryview), which pyvips accepts for zero-copy access. Sincemaskremains in scope for the duration of the call, this is safe.♻️ Use buffer directly
write_big_tiff( image=pyvips.Image.new_from_memory( - data=mask.tobytes(), width=width, height=height, bands=1, format="uchar" + data=mask.data, width=width, height=height, bands=1, format="uchar" ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@preprocessing/tile_masks.py` around lines 79 - 86, Replace the zero-copy-violating call that passes mask.tobytes() into pyvips.Image.new_from_memory inside write_big_tiff with the numpy buffer view mask.data so pyvips can consume the array without an intermediate copy; update the call site where pyvips.Image.new_from_memory(data=mask.tobytes(), ...) is used to pass data=mask.data, leaving width, height, bands and format the same and ensuring mask remains in scope for the duration of the call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@configs/preprocessing/tile_masks.yaml`:
- Around line 3-5: The YAML config has a dangerous silent default: change the
downsample entry in configs/preprocessing/tile_masks.yaml from "downsample: 1"
to either make it required (downsample: ???) or set the OOM-safe default
"downsample: 8" so new experiments don't inherit an unsafe 1× value; ensure this
aligns with the Ray task memory budget used in the process_slide logic
(process_slide) and update tests/docs if they assume a 1× default.
In `@preprocessing/tile_masks.py`:
- Line 83: The output filename currently uses the input basename
(Path(output_dir, slide_path.with_suffix(".tiff").name)) which can collide
across sources; update the code that constructs the output Path to use the
unique slide["id"] as the filename stem (e.g., build Path(output_dir,
slide["id"] + ".tiff") or equivalent) so artifacts written by the tiling
pipeline (where slide and slide_path are in scope) cannot overwrite each other
before log_artifacts runs.
---
Nitpick comments:
In `@preprocessing/tile_masks.py`:
- Around line 138-141: The current script calls ray.init(), main(), and
ray.shutdown() sequentially, which can skip ray.shutdown() if main() raises;
modify the module-level block so ray.init() is called before a try and
ray.shutdown() is invoked in a finally block to guarantee cleanup even on
exceptions — specifically wrap the calls around main() (the main function
symbol) so that ray.shutdown() always runs regardless of errors from main() or
any worker functions like process_items.
- Around line 109-120: The fallback empty DataFrame used when building items
uses pd.DataFrame(columns=["x","y"]) which creates object dtypes and can break
numeric operations in _draw_tile_outlines; change the fallback to an empty
DataFrame with numeric dtypes (e.g. pd.DataFrame({"x":
pd.Series(dtype=tiles["x"].dtype), "y": pd.Series(dtype=tiles["y"].dtype)}) or
explicit np.int64/np.float64) or alternatively filter out slides with no tiles
before constructing items so _draw_tile_outlines never receives object-typed
arrays; update the code that builds tiles_by_slide/items to use this typed empty
DataFrame (referencing tiles_by_slide, items and _draw_tile_outlines).
- Around line 79-86: Replace the zero-copy-violating call that passes
mask.tobytes() into pyvips.Image.new_from_memory inside write_big_tiff with the
numpy buffer view mask.data so pyvips can consume the array without an
intermediate copy; update the call site where
pyvips.Image.new_from_memory(data=mask.tobytes(), ...) is used to pass
data=mask.data, leaving width, height, bands and format the same and ensuring
mask remains in scope for the duration of the call.
🪄 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: 28c46b3c-a5ea-41a6-9dd8-3f8757e9c223
📒 Files selected for processing (5)
configs/data/dataset.yamlconfigs/experiment/preprocessing/tile_masks_05mpp.yamlconfigs/preprocessing/tile_masks.yamlpreprocessing/tile_masks.pyscripts/submit_tile_masks.py
…rride Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use dict[str, Any] instead of dict[str, object] so arithmetic on slide fields type-checks, and add an explicit annotation on items so process_items can infer its type parameter.
…y WSI stem Replace the local numpy-based outline drawing with the library's tile_mask helper, and name output TIFFs by the WSI stem so masks map directly to their source slides. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
preprocessing/tile_masks.py (1)
43-49:⚠️ Potential issue | 🟠 MajorUse a unique output stem here.
Path(slide["wsi_path"]).stemcan still collide across sources that share the same basename, which means one TIFF can overwrite another beforelog_artifacts()uploads the directory. The slide/WSI identifier-based naming described in the summary is not reflected in this line yet.🛠️ Suggested fix
- path=Path(output_dir, f"{Path(slide['wsi_path']).stem}.tiff"), + path=Path(output_dir, f"{slide['id']}.tiff"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@preprocessing/tile_masks.py` around lines 43 - 49, The output filename currently uses Path(slide["wsi_path"]).stem which can collide; change the path passed into write_big_tiff so the TIFF stem includes a unique slide/WSI identifier (e.g., slide["id"] or slide.get("wsi_id")/slide.get("source") ) alongside the basename to avoid overwrite; update the path expression used in the write_big_tiff call (the path argument referencing output_dir and Path(slide['wsi_path']).stem) to build a filename like "<basename>_<unique_id>.tiff" and add a safe fallback to the original stem if the identifier key is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@preprocessing/tile_masks.py`:
- Around line 24-39: The code truncates by using integer floor division
everywhere which can shrink the canvas and misalign edge tiles; update scaling
so coordinates and bounds use a consistent strategy: compute scaled tile
coordinates with floor (scaled_tiles via slide_tiles.assign x/y =
floor(x/downsample)), but compute mask tile_extent and size using ceil on
(slide["tile_extent_x"]/downsample, slide["tile_extent_y"]/downsample) and
(slide["extent_x"]/downsample, slide["extent_y"]/downsample]) respectively, then
pass those ceil-derived sizes into tile_mask so the mask fully covers the source
slide without shifting outer tiles.
- Around line 102-105: The current __main__ block calls ray.init(); main();
ray.shutdown() but ray.shutdown() only runs on the happy path—wrap the main()
invocation in a try/finally so Ray is always cleaned up: call ray.init() first,
then try: main(), finally: ray.shutdown(); update the __main__ block that
references ray.init(), main(), and ray.shutdown() accordingly to guarantee
cleanup on exceptions.
---
Duplicate comments:
In `@preprocessing/tile_masks.py`:
- Around line 43-49: The output filename currently uses
Path(slide["wsi_path"]).stem which can collide; change the path passed into
write_big_tiff so the TIFF stem includes a unique slide/WSI identifier (e.g.,
slide["id"] or slide.get("wsi_id")/slide.get("source") ) alongside the basename
to avoid overwrite; update the path expression used in the write_big_tiff call
(the path argument referencing output_dir and Path(slide['wsi_path']).stem) to
build a filename like "<basename>_<unique_id>.tiff" and add a safe fallback to
the original stem if the identifier key is missing.
🪄 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: f1fc815e-6f12-49c7-886e-63dba39fb206
📒 Files selected for processing (3)
configs/experiment/preprocessing/tile_masks_05mpp.yamlconfigs/preprocessing/tile_masks.yamlpreprocessing/tile_masks.py
✅ Files skipped from review due to trivial changes (1)
- configs/preprocessing/tile_masks.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- configs/experiment/preprocessing/tile_masks_05mpp.yaml
The tiling run's slides parquet uses 'path' (from read_slides), not 'wsi_path' which only exists upstream in wsi_mapping output. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Floor on extent_x/y could shrink the canvas by up to downsample-1 px, clipping the rightmost/bottommost tile outline. Use ceil for size while keeping floor for tile_extent and tile coordinates so outlines still tile cleanly at the scaled stride. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/submit_tile_masks.py (1)
12-16: Consider pinning repository revision for reproducible runs.Cloning the default branch at runtime can change behavior between runs. Pinning a branch/tag/commit makes MLflow artifacts reproducible.
♻️ Suggested update
script=[ - "git clone https://github.com/RationAI/tissue-classification.git workdir", + "git clone --depth 1 --branch <branch-or-tag> https://github.com/RationAI/tissue-classification.git workdir", "cd workdir", + "git checkout <commit-sha>", "uv sync", "uv run python -m preprocessing.tile_masks +experiment=...", ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/submit_tile_masks.py` around lines 12 - 16, The git clone step "git clone https://github.com/RationAI/tissue-classification.git workdir" currently checks out the default branch, which can break reproducibility; change the script to pin a revision by accepting an environment variable or parameter (e.g., REPO_REV) and run either "git clone --branch <TAG> --single-branch ... workdir" or "git clone ... workdir && cd workdir && git checkout <REV>" using that value (with a safe default tag/commit), then proceed to run "uv run python -m preprocessing.tile_masks"; ensure the update uses the same command symbol names so the clone step deterministically checks out a specific branch/tag/commit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/submit_tile_masks.py`:
- Around line 12-16: The git clone step "git clone
https://github.com/RationAI/tissue-classification.git workdir" currently checks
out the default branch, which can break reproducibility; change the script to
pin a revision by accepting an environment variable or parameter (e.g.,
REPO_REV) and run either "git clone --branch <TAG> --single-branch ... workdir"
or "git clone ... workdir && cd workdir && git checkout <REV>" using that value
(with a safe default tag/commit), then proceed to run "uv run python -m
preprocessing.tile_masks"; ensure the update uses the same command symbol names
so the clone step deterministically checks out a specific branch/tag/commit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d56801d6-f92a-44ee-a45b-1ef25159c8c3
📒 Files selected for processing (3)
configs/preprocessing/tile_masks.yamlpreprocessing/tile_masks.pyscripts/submit_tile_masks.py
🚧 Files skipped from review as they are similar to previous changes (2)
- configs/preprocessing/tile_masks.yaml
- preprocessing/tile_masks.py
ejdam87
reviewed
May 2, 2026
ejdam87
reviewed
May 2, 2026
Replace Ray remote dispatch and process_items with a plain tqdm loop. Also simplify iterrows to to_dict(orient="records"). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ejdam87
previously approved these changes
May 4, 2026
vejtek
previously approved these changes
May 4, 2026
ejdam87
approved these changes
May 5, 2026
vejtek
approved these changes
May 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements a preprocessing pipeline that generates downsampled TIFF masks
visualizing tile positions for each slide, using vectorized NumPy outline
drawing and Ray for parallel processing.
Changes
preprocessing/tile_masks.py— core pipeline: draws tile outlines as auint8 mask via vectorized NumPy indexing, downsamples coordinates and
metadata proportionally, writes output as a BigTIFF with correct MPP via
pyvips, and logs artifacts to MLflow
configs/preprocessing/tile_masks.yaml— pipeline config (downsample,artifact paths, concurrency)
configs/experiment/preprocessing/tile_masks_05mpp.yaml— experimentconfig for 0.5 MPP slides
configs/data/dataset.yaml— addedtiling_run_idMLflow artifact referencescripts/submit_tile_masks.py— cluster submission scriptNotes
budget; lowering it risks OOM on large slides
correctly
Summary by CodeRabbit
New Features
Chores