Don't require pipeline updates when setting output schema #5181
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
a502493 to
4e0a5bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
fiftyone/factory/repos/delegated_operation.py (1)
187-188: Simplify metadata initializationInstead of using
setattrwith a constant value, use direct assignment for better readability.- else: - setattr(op, "metadata", {}) + else: + op.metadata = {}🧰 Tools
🪛 Ruff (0.7.0)
188-188: Do not call
setattrwith a constant attribute value. It is not any safer than normal property access.Replace
setattrwith assignment(B010)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
fiftyone/factory/repos/delegated_operation.py(1 hunks)tests/unittests/operators/delegated_tests.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.7.0)
fiftyone/factory/repos/delegated_operation.py
188-188: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
🔇 Additional comments (3)
fiftyone/factory/repos/delegated_operation.py (1)
Line range hint 301-304: Verify MongoDB compatibility for empty schema
The code now sets metadata.outputs_schema directly. Let's verify this works with MongoDB < 5.
✅ Verification successful
Empty schema update is safe for MongoDB < 5
The code change to directly set metadata.outputs_schema as an empty object ({}) is safe for all MongoDB versions. The codebase shows consistent handling of empty objects across MongoDB operations, and there's no version-specific handling required for this case.
Key observations:
- The code has a fallback that ensures
outputs_schemais always an empty object ({}) if no schema is available - Empty objects are a fundamental MongoDB data type supported since the earliest versions
- The codebase already uses empty objects in other MongoDB operations without version checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check MongoDB version and test empty object handling
# Check MongoDB version
mongosh --eval "db.version()" --quiet
# Test empty object handling
mongosh --eval '
try {
db.test.insertOne({"metadata": {"outputs_schema": {}}});
print("Empty object test passed");
} catch (e) {
print("Empty object test failed:", e.message);
}
' --quiet
Length of output: 443
Script:
#!/bin/bash
# Let's analyze the MongoDB interaction code to understand compatibility requirements
# Search for MongoDB version checks or compatibility handling
rg -A 5 "mongodb" --glob "*.py"
# Look for MongoDB client initialization and version handling
ast-grep --pattern 'MongoClient($$$)'
# Search for MongoDB schema validation or outputs_schema handling
rg -A 5 "outputs_schema" --glob "*.py"
Length of output: 70252
🧰 Tools
🪛 Ruff (0.7.0)
180-180: Use kwargs.get("delegation_target") instead of kwargs.get("delegation_target", None)
Replace kwargs.get("delegation_target", None) with kwargs.get("delegation_target")
(SIM910)
182-182: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
184-184: Use kwargs.get("metadata") instead of kwargs.get("metadata", None)
Replace kwargs.get("metadata", None) with kwargs.get("metadata")
(SIM910)
186-186: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
188-188: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
tests/unittests/operators/delegated_tests.py (2)
215-215: LGTM! Test assertion aligns with MongoDB compatibility changes.
The updated assertion validates that metadata defaults to an empty dictionary instead of None, which aligns with the PR's goal of ensuring compatibility with MongoDB versions below 5.
Let's verify that all similar assertions in the test file are consistent:
✅ Verification successful
All metadata assertions in the test file are consistent with the changes
The verification shows that all metadata-related assertions in the test file are properly aligned with the MongoDB compatibility changes:
- One assertion verifies empty metadata:
self.assertEqual(doc.metadata, {}) - Another assertion verifies custom metadata:
self.assertEqual(doc2.metadata, doc2_metadata)
There are no remaining assertions expecting None for metadata, confirming that the changes are consistent throughout the test file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining assertions that might expect None for metadata
rg -A 2 -B 2 'assert.*metadata.*None' tests/unittests/operators/delegated_tests.py
Length of output: 84
Script:
#!/bin/bash
# Let's check all metadata-related assertions in the test file
rg -A 2 -B 2 'assert.*metadata' tests/unittests/operators/delegated_tests.py
# Also check for any other metadata comparisons
rg -A 2 -B 2 'metadata.*==' tests/unittests/operators/delegated_tests.py
Length of output: 633
Line range hint 1-1: Verify the intentional removal of test_output_schema_null_metadata.
The removal of the test method aligns with the new behavior where metadata is never None. However, this represents a breaking change in the API.
Let's check if this change is documented:
Consider documenting this breaking change in the changelog or migration guide to help users update their code that might be explicitly setting metadata to None.
On
develop, running a delegated operation with an output schema like this one would fail on MongoDB <5 with the following error:The reason was because line 337 was attempting an update aggregation like this:
and sadly this part:
is not supported on MongoDB < 5.
The solution is to ensure that the
metadatakey always exists. Then we don't need theneeds_pipeline_updateflag! 🎉Summary by CodeRabbit
New Features
Bug Fixes
Tests