Fix recursive parsing of embedded document field kwargs #6609

brimoor · 2025-11-21T16:40:33Z

Release Notes

fixed a bug in foo.get_implied_field_kwargs() when parsing embedded documents with >=2 levels of nesting

Tested by

import fiftyone as fo
import fiftyone.core.odm as foo

value = [
    fo.DynamicEmbeddedDocument(animal=fo.Detection(label="cat", instance=fo.Instance())),
    fo.DynamicEmbeddedDocument(animal=fo.Detection(label="dog", instance=fo.Instance())),
]

# previously failed, now works
kwargs = foo.get_implied_field_kwargs(value, dynamic=True)

Summary by CodeRabbit

Improvements
- More accurate display of embedded document types in field listings.
- Improved initialization and recursive handling of nested embedded document fields for more consistent schema detection and faster lookups.
Tests
- Added unit tests verifying dynamic embedded-list field schema behavior and correctness.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-11-21T16:41:17Z

Walkthrough

This pull request normalizes embedded document field definitions and adjusts an embedded-list field string representation. It adds a helper _init_embedded_doc_fields() that recursively converts field["fields"] from a list into a dict keyed by field names and applies this normalization during embedding merge operations. Separately, EmbeddedDocumentListField.__str__ was changed to show the embedded document type class name for the second element in its printed pair instead of the inner field's class name.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Merge as embedding merge
participant Init as _init_embedded_doc_fields()
participant Acc as accumulator

Merge->>Init: receive new EmbeddedDocumentField (field)
note right of Init `#E8F5E9`: recursively traverse\nfield["fields"]
Init-->>Init: convert lists -> dict by field name\ninitialize nested EmbeddedDocumentField entries
Init-->>Merge: return normalized field (fields as dict)
Merge->>Acc: add/update normalized field in accumulator
Acc-->>Merge: updated accumulator state

mermaid
sequenceDiagram
participant StrCall as EmbeddedDocumentListField.str()
participant Field as self.field
participant DocType as self.field.document_type

StrCall->>Field: read inner field
Field->>DocType: obtain document_type
StrCall-->>StrCall: format pair using DocType class name (not Field class)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–25 minutes

Verify recursive termination and handling of empty or malformed fields in _init_embedded_doc_fields().
Confirm no other code paths expect field["fields"] to remain a list; search/verify call sites.
Inspect merge logic to ensure normalized dicts preserve ordering/semantics required elsewhere.
Validate the __str__ change for EmbeddedDocumentListField does not break logging/tests that assert exact string output.
Files/areas to pay extra attention to:
- fiftyone/core/odm/utils.py (new helper and merge changes)
- fiftyone/core/fields.py (string representation change)
- tests/unittests/dataset_tests.py (new/duplicate tests for dynamic embedded lists)

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description is incomplete. It lacks the 'What changes are proposed' section and does not formally indicate whether this is a user-facing change per the template.	Add a 'What changes are proposed' section explaining the fix, explicitly check the user-facing change checkbox, and add a 'How is this patch tested' section documenting the test approach.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main fix: recursive parsing of embedded document field kwargs.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch bugfix/get-implied-field-kwargs

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2f7e0bb and 2e3dad4.

📒 Files selected for processing (1)

tests/unittests/dataset_tests.py (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

tests/unittests/dataset_tests.py (2)

fiftyone/core/dataset.py (2)

add_sample (3259-3294)

get_field_schema (1364-1423)

fiftyone/core/odm/mixins.py (1)

get_field_schema (208-275)

🪛 Pylint (4.0.3)

tests/unittests/dataset_tests.py

[convention] 6969-6969: Missing function or method docstring

(C0116)

[convention] 6969-6969: Method name "test_dynamic_embedded_list_fields" doesn't conform to 'a-z_$' pattern

(C0103)

🪛 Ruff (0.14.5)

tests/unittests/dataset_tests.py

6998-6998: Use a regular assert instead of unittest-style assertIn