feat(fiftyone-odm): TEAMS-4199 automated fcv handling #5639
Conversation
WalkthroughThis pull request introduces updates to the installation documentation for FiftyOne, reflecting a change in the required Python version to 3.9 - 3.12 and adding a note about automatic management of MongoDB feature compatibility versions starting from FiftyOne version 1.5.0. Additionally, two new constants related to MongoDB compatibility are defined, and several new functions for managing the feature compatibility version (FCV) are implemented in the database module. A new test suite is also added to validate the FCV update logic. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
docs/source/getting_started/install.rst (1)
217-225: Correct minor typos in documentation note
Fix the spelling of "conection" → "connection" and "FiftyOnes" → "FiftyOne's".Apply this diff to correct the typos:
- :ref:`MongoDB conection <configuring-mongodb-connection>` - outside of FiftyOnes administrative management. + :ref:`MongoDB connection <configuring-mongodb-connection>` + outside of FiftyOne's administrative management.tests/unittests/fcv_tests.py (3)
15-25: Consider refining docstring or method naming
_get_expected_update_callcleanly builds the expected MongoDB command. You may want to make it public if it is reused or rename it to indicate it’s for testing FCV commands if that clarifies usage.
26-87:test_update_fcv_success—well-structured functional test
- The subTest usage is beneficial for clarity.
- Nested
withstatements at lines 48-51 could be combined per static analysis for cleaner code.-with patch("pymongo.MongoClient") as mock_client: - with patch("fiftyone.core.odm.database._get_logger") as mock_get_logger: +with patch("pymongo.MongoClient") as mock_client, \ + patch("fiftyone.core.odm.database._get_logger") as mock_get_logger:🧰 Tools
🪛 Ruff (0.8.2)
45-45: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
48-51: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
45-45: Remove unusedmock_db_serviceassignments
Ruff flagged these lines wheremock_db_serviceis assigned but never used.- mock_db_service = {}(Apply this removal for each of the flagged lines shown above.)
Also applies to: 98-98, 153-153, 208-208, 262-262, 282-282, 321-321, 362-362, 401-401, 439-439, 477-477
🧰 Tools
🪛 Ruff (0.8.2)
45-45: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/source/getting_started/install.rst(1 hunks)fiftyone/constants.py(1 hunks)fiftyone/core/odm/database.py(4 hunks)tests/unittests/fcv_tests.py(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/unittests/fcv_tests.py (1)
fiftyone/core/odm/database.py (1)
_update_fc_version(1753-1817)
🪛 Ruff (0.8.2)
tests/unittests/fcv_tests.py
45-45: Local variable mock_db_service is assigned to but never used
Remove assignment to unused variable mock_db_service
(F841)
48-51: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
98-98: Local variable mock_db_service is assigned to but never used
Remove assignment to unused variable mock_db_service
(F841)
153-153: Local variable mock_db_service is assigned to but never used
Remove assignment to unused variable mock_db_service
(F841)
208-208: Local variable mock_db_service is assigned to but never used
Remove assignment to unused variable mock_db_service
(F841)
262-262: Local variable mock_db_service is assigned to but never used
Remove assignment to unused variable mock_db_service
(F841)
282-282: Local variable mock_db_service is assigned to but never used
Remove assignment to unused variable mock_db_service
(F841)
321-321: Local variable mock_db_service is assigned to but never used
Remove assignment to unused variable mock_db_service
(F841)
362-362: Local variable mock_db_service is assigned to but never used
Remove assignment to unused variable mock_db_service
(F841)
401-401: Local variable mock_db_service is assigned to but never used
Remove assignment to unused variable mock_db_service
(F841)
439-439: Local variable mock_db_service is assigned to but never used
Remove assignment to unused variable mock_db_service
(F841)
477-477: Local variable mock_db_service is assigned to but never used
Remove assignment to unused variable mock_db_service
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: lint / eslint
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: build
🔇 Additional comments (15)
fiftyone/constants.py (1)
105-106: Constants appear correct and well-named
No issues with these constants, and they cleanly convey their meaning.tests/unittests/fcv_tests.py (11)
1-14: Imports and initial setup look good
No issues noted in these import statements, and theunittest.mockusage is appropriate.
88-140:test_update_fcv_success_minor_and_patch_versions—test coverage looks good
This test effectively verifies FCV updates when MongoDB versions differ by minor or patch increments. Implementation is logically consistent.🧰 Tools
🪛 Ruff (0.8.2)
98-98: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
142-196:test_update_fcv_success_fcv_less_than_min_version—robust boundary check
Covers an edge case of server version meeting the minimum while FCV is below it. Nicely done.🧰 Tools
🪛 Ruff (0.8.2)
153-153: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
198-247:test_update_fcv_log_minimum_version—good use of assertion checks
Ensures no unintended upgrade call for identical oldest supported version. Helpful for preventing regressions.🧰 Tools
🪛 Ruff (0.8.2)
208-208: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
255-271:test_connection_error—connection error scenario
Raising aConnectionErrorensures consistent error handling across validation calls. Looks correct.🧰 Tools
🪛 Ruff (0.8.2)
262-262: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
273-311:test_version_diff_warning—validates large version gap
Properly triggers a warning for major version differences beyond the allowed delta. Implementation aligns with design.🧰 Tools
🪛 Ruff (0.8.2)
282-282: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
313-351:test_fcv_greater_than_server_version_warning—thorough coverage
Checks for FCV outpacing the server version. The test logic sufficiently covers and logs this mismatch scenario.🧰 Tools
🪛 Ruff (0.8.2)
321-321: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
353-391:test_oldest_supported_version_warning—ensures no unwanted upgrade
This test enforces that only logs occur when both FCV and server version are at the lowest support level. Good negative test.🧰 Tools
🪛 Ruff (0.8.2)
362-362: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
393-429:test_update_fc_version_operation_failure—valid OperationFailure coverage
Confirms that errors are logged and no unhandled exceptions propagate. Correct approach for robust error handling.🧰 Tools
🪛 Ruff (0.8.2)
401-401: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
431-467:test_update_fc_version_pymongo_failure—verifying PyMongoError
Similar coverage for a different exception type. Good for ensuring broad reliability.🧰 Tools
🪛 Ruff (0.8.2)
439-439: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
469-523:test_no_action_unless_managed—prevents upgrades when _db_service is None
Successfully tests scenario in which FiftyOne does not manage the database. Implementation is consistent.🧰 Tools
🪛 Ruff (0.8.2)
477-477: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
fiftyone/core/odm/database.py (3)
1653-1681:_get_fcv_and_version—solid retrieval of FCV and server version
- Properly raises
ConnectionErroronServerSelectionTimeoutError.- Implementation is concise and clear.
1683-1751:_is_fcv_upgradeable—comprehensive checks
Handles multiple mismatch scenarios carefully (FCV > server, large version gaps, and oldest supported version). Logs are explicit.
1753-1817:_update_fc_version—key logic to upgrade FCV
Implementation covers warnings, errors, and major version check thoroughly. Reliance on_db_serviceis consistent with the design.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
tests/unittests/fcv_tests.py (12)
45-45: Fix unused variable assignment.The
mock_db_servicevariable is assigned but never used. The patched_db_serviceis already handled by the decorator.- mock_db_service = {} + # Database service should exist for this test🧰 Tools
🪛 Ruff (0.8.2)
45-45: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
48-53: Simplify nested context managers.Multiple nested
withstatements can be combined for improved readability.- with self.subTest( - server_version=server_version, fc_version=fc_version - ): - with patch("pymongo.MongoClient") as mock_client, patch( - "fiftyone.core.odm.database._get_logger" - ) as mock_get_logger: + with self.subTest( + server_version=server_version, fc_version=fc_version + ), patch("pymongo.MongoClient") as mock_client, patch( + "fiftyone.core.odm.database._get_logger" + ) as mock_get_logger:🧰 Tools
🪛 Ruff (0.8.2)
48-53: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
98-98: Fix unused variable assignment.Similar to earlier instances, the
mock_db_servicevariable is assigned but never used.- mock_db_service = {} + # Database service should exist for this test🧰 Tools
🪛 Ruff (0.8.2)
98-98: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
153-153: Fix unused variable assignment.Same issue with unused
mock_db_servicevariable.- mock_db_service = {} + # Database service should exist for this test🧰 Tools
🪛 Ruff (0.8.2)
153-153: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
208-208: Fix unused variable assignment.Same issue with unused
mock_db_servicevariable.- mock_db_service = {} + # Database service should exist for this test🧰 Tools
🪛 Ruff (0.8.2)
208-208: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
262-262: Fix unused variable assignment.Same issue with unused
mock_db_servicevariable.- mock_db_service = {} + # Database service should exist for this test🧰 Tools
🪛 Ruff (0.8.2)
262-262: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
282-282: Fix unused variable assignment.Same issue with unused
mock_db_servicevariable.- mock_db_service = {} + # Database service should exist for this test🧰 Tools
🪛 Ruff (0.8.2)
282-282: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
321-321: Fix unused variable assignment.Same issue with unused
mock_db_servicevariable.- mock_db_service = {} + # Database service should exist for this test🧰 Tools
🪛 Ruff (0.8.2)
321-321: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
362-362: Fix unused variable assignment.Same issue with unused
mock_db_servicevariable.- mock_db_service = {} + # Database service should exist for this test🧰 Tools
🪛 Ruff (0.8.2)
362-362: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
401-401: Fix unused variable assignment.Same issue with unused
mock_db_servicevariable.- mock_db_service = {} + # Database service should exist for this test🧰 Tools
🪛 Ruff (0.8.2)
401-401: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
439-439: Fix unused variable assignment.Same issue with unused
mock_db_servicevariable.- mock_db_service = {} + # Database service should exist for this test🧰 Tools
🪛 Ruff (0.8.2)
439-439: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
1-523: Overall test suite is comprehensive and robust.The test suite is well-structured and covers a wide range of scenarios for the FCV update functionality, including success cases, edge cases, and various error conditions. The mocking approach effectively isolates the tests from actual MongoDB dependencies.
Consider adding assertions for specific functions being called (like
server_info()) rather than just focusing on the final command calls. This would provide even more validation of the expected code paths.🧰 Tools
🪛 Ruff (0.8.2)
45-45: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
48-53: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
98-98: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
153-153: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
208-208: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
262-262: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
282-282: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
321-321: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
362-362: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
401-401: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
439-439: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
477-477: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/source/getting_started/install.rst(1 hunks)tests/unittests/fcv_tests.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/source/getting_started/install.rst
🧰 Additional context used
🧬 Code Definitions (1)
tests/unittests/fcv_tests.py (1)
fiftyone/core/odm/database.py (1)
_update_fc_version(1753-1817)
🪛 Ruff (0.8.2)
tests/unittests/fcv_tests.py
45-45: Local variable mock_db_service is assigned to but never used
Remove assignment to unused variable mock_db_service
(F841)
48-53: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
98-98: Local variable mock_db_service is assigned to but never used
Remove assignment to unused variable mock_db_service
(F841)
153-153: Local variable mock_db_service is assigned to but never used
Remove assignment to unused variable mock_db_service
(F841)
208-208: Local variable mock_db_service is assigned to but never used
Remove assignment to unused variable mock_db_service
(F841)
262-262: Local variable mock_db_service is assigned to but never used
Remove assignment to unused variable mock_db_service
(F841)
282-282: Local variable mock_db_service is assigned to but never used
Remove assignment to unused variable mock_db_service
(F841)
321-321: Local variable mock_db_service is assigned to but never used
Remove assignment to unused variable mock_db_service
(F841)
362-362: Local variable mock_db_service is assigned to but never used
Remove assignment to unused variable mock_db_service
(F841)
401-401: Local variable mock_db_service is assigned to but never used
Remove assignment to unused variable mock_db_service
(F841)
439-439: Local variable mock_db_service is assigned to but never used
Remove assignment to unused variable mock_db_service
(F841)
477-477: Local variable mock_db_service is assigned to but never used
Remove assignment to unused variable mock_db_service
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: modified-files
- GitHub Check: build
🔇 Additional comments (14)
tests/unittests/fcv_tests.py (14)
1-13: Imports look good!The imports are appropriate for a test file, including unittest, mock utilities, pymongo error types, and the function under test.
15-24: Helper method is well implemented.The
_get_expected_update_callhelper method cleanly abstracts the construction of the MongoDB command, handling the conditional addition of theconfirmflag based on version requirements.
26-87: Test cases thoroughly verify successful FCV updates.The test comprehensively verifies the FCV update functionality across multiple version increments, ensuring proper command execution and logging.
🧰 Tools
🪛 Ruff (0.8.2)
45-45: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
48-53: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
88-140: Test case correctly verifies minor and patch version handling.This test ensures that MongoDB's minor and patch version differences are properly handled during FCV updates, including verifying appropriate warning messages.
🧰 Tools
🪛 Ruff (0.8.2)
98-98: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
142-195: Test case effectively handles edge case with FCV below minimum version.The test properly verifies the behavior when the FCV is below the minimum supported version while the server version is at the minimum. This is a good edge case to cover.
🧰 Tools
🪛 Ruff (0.8.2)
153-153: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
197-254: Good test for minimum version edge case.Excellent test case that verifies when both versions are at minimum support level, no action is taken beyond logging warnings.
🧰 Tools
🪛 Ruff (0.8.2)
208-208: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
255-271: Connection error handling test is well implemented.The test correctly verifies that connection errors are properly propagated as ConnectionError exceptions.
🧰 Tools
🪛 Ruff (0.8.2)
262-262: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
272-311: Version difference warning test is well implemented.The test properly verifies that warnings are logged when the MongoDB server version is significantly ahead of the FCV.
🧰 Tools
🪛 Ruff (0.8.2)
282-282: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
312-350: Test case for FCV greater than server version is thorough.Good validation of warning behavior when FCV exceeds server version, which is an important edge case to handle.
🧰 Tools
🪛 Ruff (0.8.2)
321-321: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
351-391: Great test for minimum supported version warning.The test effectively verifies that appropriate warnings are generated when running the oldest supported MongoDB version.
🧰 Tools
🪛 Ruff (0.8.2)
362-362: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
392-428: Operation failure test is well implemented.The test properly verifies error logging when the FCV update operation fails, including checking for the correct error message.
🧰 Tools
🪛 Ruff (0.8.2)
401-401: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
430-466: PyMongo failure test is well implemented.The test effectively verifies error logging behavior for general PyMongo errors during FCV updates.
🧰 Tools
🪛 Ruff (0.8.2)
439-439: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
468-523: Test for unmanaged instances is well implemented.This test properly verifies that only logging occurs (no actual updates) when the database instance is not managed by FiftyOne.
🧰 Tools
🪛 Ruff (0.8.2)
477-477: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
477-477:✅ Verification successful
Intentional assignment of None to mock_db_service.
This is the only test where assigning to
mock_db_serviceis intentional and correct, as it tests the behavior when the service isNone. However, ensure this assignment actually affects the patched value.
🏁 Script executed:
#!/bin/bash # Verifying how patched variables work with assignments # Let's check if there are any similar patterns in the codebase rg -A 2 -B 2 "mock_\w+ = None" --glob "tests/**/*.py"Length of output: 388
Review Verification: Confirmed Intentional Assignment of None in
fcv_tests.pyThe search confirms that the assignment of
Nonetomock_db_serviceintests/unittests/fcv_tests.pyis unique to this test and matches the intended behavior. Please ensure that this assignment correctly propagates to the patched mechanism in your test setup.🧰 Tools
🪛 Ruff (0.8.2)
477-477: Local variable
mock_db_serviceis assigned to but never usedRemove assignment to unused variable
mock_db_service(F841)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
fiftyone/core/odm/database.py (2)
1683-1750: Comprehensive FCV upgrade eligibility checks.The function thoroughly handles all edge cases with appropriate warning messages. It correctly prevents upgrades in potentially problematic scenarios.
Consider adding a return type annotation for consistency with other functions:
-def _is_fcv_upgradeable(fc_version: Version, server_version: Version) -> bool: +def _is_fcv_upgradeable(fc_version: Version, server_version: Version) -> bool:
1753-1817: Solid implementation of FCV update mechanism.The function correctly handles the MongoDB server version-specific requirements (like the
confirmflag for 7.0+) and provides informative error messages if the update fails.Consider adding a return type annotation for consistency:
-def _update_fc_version(client: pymongo.MongoClient): +def _update_fc_version(client: pymongo.MongoClient) -> None:
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/source/getting_started/install.rst(3 hunks)fiftyone/core/odm/database.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/source/getting_started/install.rst
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test / test-app
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: build
🔇 Additional comments (3)
fiftyone/core/odm/database.py (3)
15-15: Proper imports added for new functionality.The addition of the
Tupletype and specific MongoDB error classes is necessary for the new feature compatibility version (FCV) handling functions.Also applies to: 25-31
235-236: Well-integrated FCV update mechanism.The code properly conditionally calls the new FCV update mechanism only when both database validation and admin permissions are enabled, ensuring it won't affect users connecting to external databases.
1653-1680: Well-implemented FCV and version retrieval function.Good type annotations, error handling, and clear docstring. The function properly fetches both the feature compatibility version and server version, converting them to Version objects for comparison.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unittests/fcv_tests.py (1)
303-307: Minor spelling fix.In the docstring referencing “feature compatability,” please correct “compatability” to “compatibility.”
- """Tests the warning that's generated in the event that - the feature compatability version is ahead of the server version. + """Tests the warning that's generated in the event that + the feature compatibility version is ahead of the server version.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
fiftyone/constants.py(1 hunks)fiftyone/core/odm/database.py(5 hunks)tests/unittests/fcv_tests.py(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
tests/unittests/fcv_tests.py (1)
fiftyone/core/odm/database.py (1)
_update_fc_version(1753-1817)
fiftyone/core/odm/database.py (1)
fiftyone/core/dataset.py (1)
version(687-691)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: build / build
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test / test-app
- GitHub Check: e2e / test-e2e
- GitHub Check: lint / eslint
- GitHub Check: build
🔇 Additional comments (7)
fiftyone/constants.py (1)
104-106: All constants appear well-defined and aligned with the intended usage.tests/unittests/fcv_tests.py (1)
1-503: Comprehensive test coverage.These tests thoroughly validate FCV upgrade scenarios, error handling, and logging behavior. Great use of mocks to cover various MongoDB states.
fiftyone/core/odm/database.py (5)
8-8: No functional changes detected.
15-15: Correct import for typed return values.
25-31: Consolidated imports for error handling.
235-236: Conditional FCV update is coherent.This ensures that FCV change logic only runs when both admin privileges and database validation are enabled.
1653-1818: Robust FCV management logic.These new helper functions cleanly handle FCV detection, upgrade checks, and controlled error handling. The flow is well-structured and thoroughly tested.
What changes are proposed in this pull request?
Various versions of mongoDB are being deprecated and moving to EOL. FiftyOne should be able to be updated with those changes. MongoDB has a few version numbers and, of importance, are the server version and the feature compatibility version (FCV). The server version is the version of the binary that is running while the FCV tells the database which server features to be compatible with.
With embedded databases (ones spawned by FO), we should check this FCV and, if required, increase it to the same major version as the server. For reference, see the mongodb docs.
Originally, this was the following 2 PRs:
How is this patch tested? If it is not, please explain why.
Case 1 - If database_validation=False OR database_admin=False
Background
If
database_validation=FalseORdatabase_admin=False:Test
Case 2a.1 - Using the internal/embedded mongo instance
Background
If
database_admin=TrueANDdatabase_validation=True_db_service != None, Automatically issue admin command to upgrade FCVaccording to MongoDB best practices
In this test, we will validate that this occurs IFF both settings are true and
_db_serviceis notNone.Test
First, install/run fiftyone from PyPi
Now, run FO:
Then, build from source which should trigger update
$ git clone https://github.com/voxel51/fiftyone.git \ -b afoley/teams-4199-automated-fcv-handling $ cd fiftyone $ bash install.bash -d $ export PYTHONPATH=$PWD:$PYTHONPATH # Force update the mongo binary so server version > fc version $ wget -O mongo-next.tgz \ https://fastdl.mongodb.org/linux/mongodb-linux-x86_64-debian12-7.0.18.tgz $ tar xvzf mongo-next.tgz $ find mongodb-linux-* \ -name mongod -exec cp {} \ $HOME/.venv/lib/python3.10/site-packages/fiftyone/db/bin/mongod \; $ $HOME/.venv/lib/python3.10/site-packages/fiftyone/db/bin/mongod --version db version v7.0.18 Build Info: { "version": "7.0.18", "gitVersion": "1624e181d0dac03edb34ef25b9b58a6d79969825", "openSSLVersion": "OpenSSL 3.0.15 3 Sep 2024", "modules": [], "allocator": "tcmalloc", "environment": { "distmod": "debian12", "distarch": "x86_64", "target_arch": "x86_64" } }Now, run FO:
Case 2a.2 - Using the internal/embedded mongo instance
Background
If
database_admin=TrueANDdatabase_validation=True_db_service != None, Automatically issue admin command to upgrade FCVaccording to MongoDB best practices
In this test, We will perform another forced mongo update.
In this case, we will set fiftyone db admin to false and make sure the parameters are respected.
Test
First, force upgrade mongo:
$ rm -rf mongo-next.tgz mongodb-linux-x86_64-debian12-7.0.18 $ wget -O mongo-next.tgz \ https://fastdl.mongodb.org/linux/mongodb-linux-x86_64-debian12-8.0.6.tgz $ tar xvzf mongo-next.tgz $ find mongodb-linux-* \ -name mongod -exec cp {} \ $HOME/.venv/lib/python3.10/site-packages/fiftyone/db/bin/mongod \; $ $HOME/.venv/lib/python3.10/site-packages/fiftyone/db/bin/mongod --version db version v8.0.6 Build Info: { "version": "8.0.6", "gitVersion": "80f21521ad4a3dfd5613f5d649d7058c6d46277f", "openSSLVersion": "OpenSSL 3.0.15 3 Sep 2024", "modules": [], "allocator": "tcmalloc-google", "environment": { "distmod": "debian12", "distarch": "x86_64", "target_arch": "x86_64" } }And then run FO. Note the lack of log messages:
Now, set the configuration to true and observe the upgrade:
Validating the FCV:
Case 2b - Respecting External Databases
Background
If
database_admin=TrueANDdatabase_validation=True_db_service != None, Automatically issue admin command to upgrade FCVaccording to MongoDB best practices
In this test, we will validate that FO will not manage any FCV is
this is an external database (
_db_serviceisNone).Test
In one shell, run an external mongo:
$ docker run -it --rm -p 27018:27017 -v ./db:/data/db mongo:4.4 # Run it again so FCV != server $ docker run -it --rm -p 27018:27017 -v ./db:/data/db mongo:5.0And connect to it. In this case, the logs should be shown but no actions performed (FCV is STILL on 4.4):
Verifying that mongo wasn't upgraded:
Now, disable database validation and observe that there are no logs:
See below for "how were the docs tested"
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
FiftyOne will now manage the feature compatibility version (FCV) of the FiftyOne-managed mongodb instance. This management will happen during the first connection to the database. Customers who utilize an external/standalone instance will be unaffected.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Summary by CodeRabbit
Documentation
New Features
Tests