Add image_resolver.get_for_export with collection filter support#1637
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a public ChangesImage export filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Caller
participant get_for_export
participant EmbeddingRegionResolver
participant Database
Caller->>get_for_export: Request collection export with optional filter and preload
get_for_export->>EmbeddingRegionResolver: Resolve embedding-region sample IDs
EmbeddingRegionResolver-->>get_for_export: Return matching sample IDs
get_for_export->>Database: Execute filtered image/sample query with eager-loading
Database-->>get_for_export: Return matching rows
get_for_export-->>Caller: Yield ImageSample values
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00e4524bb4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lightly_studio/src/lightly_studio/resolvers/image_resolver/get_for_export.py (1)
39-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate embedding-region resolution logic across two resolvers. Both sites implement the identical "resolve
embedding_region→region_sample_ids, thenmodel_copythe filter" sequence; extracting a shared helper avoids the two copies drifting out of sync as this logic evolves.
lightly_studio/src/lightly_studio/resolvers/image_resolver/get_for_export.py#L39-L52: extract this block into a shared helper (e.g._resolve_embedding_region_filter(session, collection_id, collection_filter)) and call it here.lightly_studio/src/lightly_studio/resolvers/image_resolver/get_all_by_collection_id.py#L103-L116: replace this block with a call to the same shared helper.♻️ Proposed shared helper
def _resolve_embedding_region_filter( session: Session, collection_id: UUID, filters: ImageFilter | None, ) -> ImageFilter | None: """Resolve an embedding-region filter to concrete sample ids, if present.""" if filters is None or filters.sample_filter is None or filters.sample_filter.embedding_region is None: return filters region_sample_ids = embedding_region_resolver.get_sample_ids_in_region( session=session, collection_id=collection_id, region=filters.sample_filter.embedding_region, ) resolved_sample_filter = filters.sample_filter.model_copy( update={"region_sample_ids": region_sample_ids} ) return filters.model_copy(update={"sample_filter": resolved_sample_filter})🤖 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 `@lightly_studio/src/lightly_studio/resolvers/image_resolver/get_for_export.py` around lines 39 - 52, Extract the duplicated embedding-region resolution logic into a shared helper, such as _resolve_embedding_region_filter, preserving the existing None and no-region behavior while returning a copied filter with resolved region_sample_ids when needed. Update get_for_export.py lines 39-52 and get_all_by_collection_id.py lines 103-116 to call the helper, with no direct logic remaining in either site.
🤖 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.
Nitpick comments:
In
`@lightly_studio/src/lightly_studio/resolvers/image_resolver/get_for_export.py`:
- Around line 39-52: Extract the duplicated embedding-region resolution logic
into a shared helper, such as _resolve_embedding_region_filter, preserving the
existing None and no-region behavior while returning a copied filter with
resolved region_sample_ids when needed. Update get_for_export.py lines 39-52 and
get_all_by_collection_id.py lines 103-116 to call the helper, with no direct
logic remaining in either site.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aad83a9a-706f-428d-b6f4-063925a93096
📒 Files selected for processing (4)
lightly_studio/src/lightly_studio/resolvers/image_resolver/__init__.pylightly_studio/src/lightly_studio/resolvers/image_resolver/get_all_by_collection_id.pylightly_studio/src/lightly_studio/resolvers/image_resolver/get_for_export.pylightly_studio/tests/resolvers/image_resolver/test_get_for_export.py
|
/review |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lightly_studio/src/lightly_studio/resolvers/image_resolver/get_for_export.py (1)
94-94: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winUse
yield_perto prevent buffering all rows in memory.Although
get_for_exportreturns a generator,session.exec(query)will execute the statement and buffer all matching ORM instances into memory before yielding the first row. For a collection export generator, this can cause Out-Of-Memory (OOM) errors on large datasets.Use
.execution_options(yield_per=...)to stream the results in memory-efficient chunks. (Note:yield_peris fully compatible with bothcontains_eagerandselectinloadin modern SQLAlchemy).⚡ Proposed fix
- return (ImageSample(row) for row in session.exec(query)) + query = query.execution_options(yield_per=1000) + return (ImageSample(row) for row in session.exec(query))🤖 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 `@lightly_studio/src/lightly_studio/resolvers/image_resolver/get_for_export.py` at line 94, Update the query execution in get_for_export to apply SQLAlchemy execution_options(yield_per=...) before session.exec, so ORM rows are streamed in chunks rather than buffered entirely; preserve the existing ImageSample generator behavior and eager-loading options.
🧹 Nitpick comments (1)
lightly_studio/src/lightly_studio/resolvers/image_resolver/get_for_export.py (1)
85-87: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnset
embedding_regionafter resolving it to prevent stale filter processing.When replacing the
embedding_regionfilter with concreteregion_sample_ids, explicitly unset theembedding_regionfield in the copied model. This cleanly discards the unresolvable polygon state and ensures that downstream query builders (likebuild_sample_ids_query) do not redundantly process it or raise unsupported-filter errors.♻️ Proposed fix
- resolved_sample_filter = sample_filter.model_copy( - update={"region_sample_ids": region_sample_ids} - ) + resolved_sample_filter = sample_filter.model_copy( + update={ + "region_sample_ids": region_sample_ids, + "embedding_region": None, + } + )🤖 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 `@lightly_studio/src/lightly_studio/resolvers/image_resolver/get_for_export.py` around lines 85 - 87, Update the copied filter construction in the export resolver so resolving embedding_region into region_sample_ids also explicitly clears embedding_region. Ensure downstream query builders such as build_sample_ids_query receive the concrete region IDs without retaining the original polygon filter.
🤖 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.
Outside diff comments:
In
`@lightly_studio/src/lightly_studio/resolvers/image_resolver/get_for_export.py`:
- Line 94: Update the query execution in get_for_export to apply SQLAlchemy
execution_options(yield_per=...) before session.exec, so ORM rows are streamed
in chunks rather than buffered entirely; preserve the existing ImageSample
generator behavior and eager-loading options.
---
Nitpick comments:
In
`@lightly_studio/src/lightly_studio/resolvers/image_resolver/get_for_export.py`:
- Around line 85-87: Update the copied filter construction in the export
resolver so resolving embedding_region into region_sample_ids also explicitly
clears embedding_region. Ensure downstream query builders such as
build_sample_ids_query receive the concrete region IDs without retaining the
original polygon filter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8f9dcfc6-077a-4581-afd7-0b403416048f
📒 Files selected for processing (3)
lightly_studio/src/lightly_studio/resolvers/image_resolver/__init__.pylightly_studio/src/lightly_studio/resolvers/image_resolver/get_for_export.pylightly_studio/tests/resolvers/image_resolver/test_get_for_export.py
🚧 Files skipped from review as they are similar to previous changes (2)
- lightly_studio/src/lightly_studio/resolvers/image_resolver/init.py
- lightly_studio/tests/resolvers/image_resolver/test_get_for_export.py
2 participants
What has changed and why?
Extracts a dedicated
get_for_exportfunction into theimage_resolvermodule to serve image export pipelines. The function returns a lazy generator ofImageSampleobjects for a given collection, applying an optionalImageFilter(including embedding-region polygon filters resolved to concrete sample IDs before the query runs).This is a follow-up to #1632, which introduced the export endpoint layer; this PR adds the resolver that backs it.
How has it been tested?
Unit tests
Did you update CHANGELOG.md?
Summary by CodeRabbit