chore(fiftyone-odm): AS-331 Automated FCV Upgrades #5441
Conversation
WalkthroughThis pull request introduces enhancements to the management of MongoDB feature compatibility versions (FCV) within FiftyOne. It includes updates to the documentation, detailing how users should manage MongoDB versions for FiftyOne versions greater than 1.3.0. Additionally, new functions are added to the database module to facilitate the retrieval and updating of the FCV, alongside comprehensive unit tests to validate these functionalities. Changes
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🔇 Additional comments (1)
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
|
![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 (3)
tests/unittests/fcv_tests.py (1)
3-7: Remove unused imports.The following imports are not used in the test file:
pymongo.errors.OperationFailurepymongo.errors.PyMongoErrorfrom pymongo.errors import ( ServerSelectionTimeoutError, - OperationFailure, - PyMongoError, )🧰 Tools
🪛 Ruff (0.8.2)
5-5:
pymongo.errors.OperationFailureimported but unusedRemove unused import
(F401)
6-6:
pymongo.errors.PyMongoErrorimported but unusedRemove unused import
(F401)
fiftyone/core/odm/database.py (2)
351-414: Extract magic numbers into named constants.Consider extracting magic numbers into named constants for better maintainability:
+# Maximum allowed difference between major versions before warning +MAX_MAJOR_VERSION_DIFF = 2 + def _is_fcv_upgradeable(fc_version: Version, server_version: Version) -> bool: - max_allowable_major_diff = ( - 2 # if greater than 2 major versions, issue warning - ) + max_allowable_major_diff = MAX_MAJOR_VERSION_DIFF
416-448: Enhance error message specificity.Consider providing more specific error messages based on the error type:
- except (OperationFailure, PyMongoError) as e: + except OperationFailure as e: _logger.error( - "Could not automatically update your database's feature " - "compatability version - %s. " - "Please set it to %s." % (str(e), bumped) + "Operation failed while updating database's feature " + "compatibility version - %s. " + "Please manually set it to %s." % (str(e), bumped) + ) + except PyMongoError as e: + _logger.error( + "MongoDB error while updating database's feature " + "compatibility version - %s. " + "Please verify your connection and manually set it to %s." % (str(e), bumped) )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/source/getting_started/install.rst(1 hunks)fiftyone/core/odm/database.py(4 hunks)tests/unittests/fcv_tests.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/unittests/fcv_tests.py
5-5: pymongo.errors.OperationFailure imported but unused
Remove unused import
(F401)
6-6: pymongo.errors.PyMongoError imported but unused
Remove unused import
(F401)
⏰ 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: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: lint / eslint
- GitHub Check: build
🔇 Additional comments (8)
tests/unittests/fcv_tests.py (5)
19-49: LGTM! Well-structured test for FCV update.The test thoroughly covers the success path for feature compatibility version updates:
- Proper mock setup
- Verification of command execution
- Warning message validation
51-60: LGTM! Good error handling test.The test effectively verifies that connection errors are properly handled and converted to ConnectionError.
61-91: LGTM! Comprehensive version difference test.The test properly validates the warning scenario when server version is significantly ahead of FCV.
92-124: LGTM! Good edge case coverage.The test properly handles the unusual case where FCV is greater than server version.
125-155: LGTM! Good minimum version test.The test properly validates the warning scenario for oldest supported MongoDB version.
fiftyone/core/odm/database.py (2)
323-349: LGTM! Well-implemented version fetching.The function is well-documented with proper type hints and error handling.
231-234: LGTM! Well-integrated FCV update logic.The FCV update logic is properly integrated with appropriate checks for database type.
docs/source/getting_started/install.rst (1)
217-225: LGTM! Clear and informative documentation.The added note effectively communicates:
- New FCV management feature in FiftyOne 1.3.0+
- Important backup recommendation
- Alternative options for users
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/unittests/fcv_tests.py (2)
16-49: Add docstring to describe test purpose and expectations.The test method would benefit from a docstring explaining what it tests and the expected behavior.
def test_update_fcv_success(self, mock_get_logger, mock_client): + """Tests successful FCV update when server version is one major version ahead. + + The test verifies that: + 1. The FCV update command is executed with the correct version + 2. A warning is logged about the version mismatch + """ # Set up the mock client and server info
51-217: Add docstrings to remaining test methods.Each test method should have a docstring explaining its purpose, test scenario, and expected behavior.
Example for one of the test methods:
def test_version_diff_warning(self, mock_get_logger, mock_client): + """Tests warning generation when server version is more than one major version ahead. + + The test verifies that: + 1. A warning is logged when server version is two major versions ahead + 2. The warning message correctly indicates manual intervention is needed + """ # Set up the mock client and server infofiftyone/core/odm/database.py (1)
413-452: Consider adding version information to error messages.The error handling is good, but the error messages could be more informative by including the current FCV version.
_logger.error( - "Operation failed while updating database's feature " - "compatibility version - %s. " - "Please manually set it to %s." % (str(e), bumped) + "Operation failed while updating database's feature " + "compatibility version from %s - %s. " + "Please manually set it to %s." % (fc_version, str(e), bumped) )
📜 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(4 hunks)tests/unittests/fcv_tests.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- 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: build / changes
- GitHub Check: e2e / test-e2e
- GitHub Check: build
🔇 Additional comments (5)
fiftyone/constants.py (1)
105-105: LGTM! Well-defined constant for MongoDB FCV management.The constant follows the naming convention and its value aligns with MongoDB's recommendation that feature compatibility versions should not differ by more than one major version.
tests/unittests/fcv_tests.py (1)
1-14: LGTM! Well-organized imports and test setup.The imports are properly organized and include all necessary components for MongoDB feature compatibility version testing.
fiftyone/core/odm/database.py (3)
231-234: LGTM! Well-placed FCV update integration.The FCV update is correctly integrated into the database connection establishment process, with appropriate checks for FiftyOne-managed databases.
323-349: LGTM! Well-implemented FCV and version retrieval.The function is well-documented with proper type hints and includes appropriate error handling for connection issues.
351-411: LGTM! Comprehensive FCV upgrade validation.The function implements thorough version comparison logic with clear warning messages for each scenario.
afoley587
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
fiftyone/core/odm/database.py (2)
352-412: Consider adding version comparison validation.While the function handles various version comparison scenarios well, it might be good to add validation for the version parameters to ensure they are not None.
def _is_fcv_upgradeable(fc_version: Version, server_version: Version) -> bool: + if fc_version is None or server_version is None: + _logger = _get_logger() + _logger.warning("Invalid version parameters provided") + return False + _logger = _get_logger()
414-454: Consider adding retry mechanism for transient errors.The function handles operation failures well, but for transient MongoDB errors, a retry mechanism might be beneficial.
def _update_fc_version(client: pymongo.MongoClient): + max_retries = 3 + retry_delay = 1 # seconds + fc_version, server_version = _get_fcv_and_version(client) _logger = _get_logger() if _is_fcv_upgradeable(fc_version, server_version): bumped = f"{server_version.major}.0" - try: - client.admin.command( - { - "setFeatureCompatibilityVersion": bumped, - "confirm": True, - } - ) - except OperationFailure as e: - _logger.error( - "Operation failed while updating database's feature " - "compatibility version - %s. " - "Please manually set it to %s." % (str(e), bumped) - ) - - except PyMongoError as e: - _logger.error( - "MongoDB error while updating database's feature " - "compatibility version - %s. " - "Please manually set it to %s." % (str(e), bumped) - ) + for attempt in range(max_retries): + try: + client.admin.command( + { + "setFeatureCompatibilityVersion": bumped, + "confirm": True, + } + ) + break + except OperationFailure as e: + _logger.error( + "Operation failed while updating database's feature " + "compatibility version - %s. " + "Please manually set it to %s." % (str(e), bumped) + ) + break # Don't retry on operation failures + except PyMongoError as e: + if attempt < max_retries - 1: + _logger.warning( + "MongoDB error on attempt %d/%d - %s. " + "Retrying in %d second(s)...", + attempt + 1, max_retries, str(e), retry_delay + ) + time.sleep(retry_delay) + else: + _logger.error( + "MongoDB error while updating database's feature " + "compatibility version - %s. " + "Please manually set it to %s." % (str(e), bumped) + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/source/getting_started/install.rst(2 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 (7)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: lint / eslint
- GitHub Check: build / changes
- GitHub Check: build
🔇 Additional comments (3)
fiftyone/core/odm/database.py (3)
15-15: LGTM! Import changes are appropriate.The new imports support type hints and error handling for the FCV management functionality.
Also applies to: 25-30
324-350: LGTM! Well-structured MongoDB version retrieval.The function properly fetches and returns both the feature compatibility version and server version with appropriate error handling.
232-235: LGTM! Well-integrated FCV update logic.The FCV update is properly guarded with conditions to ensure it only runs for FiftyOne-managed databases.
benjaminpkane
force-pushed
the
afoley/AS-331-pymongo-fcv-updates
branch
from
a115f18 to
ef83935
Compare
|
This could definitely use some battle testing if other Aloha members have time to review |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
tests/unittests/fcv_tests.py (3)
51-60: Consider adding error message verification.While the test correctly verifies that a ConnectionError is raised, it would be more robust to also verify the error message content.
with self.assertRaises(ConnectionError): - _update_fc_version(mock_client) + with self.assertRaisesRegex(ConnectionError, "Could not connect to `mongod`"): + _update_fc_version(mock_client)
158-187: Add assertion for command call count.The test should verify that the command was called exactly twice (once for getting FCV and once for the update attempt).
_update_fc_version(mock_client) -# Check that the warning is triggered for the oldest supported version +# Verify command was called twice +self.assertEqual(mock_admin.command.call_count, 2) + +# Check that the error is logged mock_logger.error.assert_any_call(
189-218: Consider adding test for successful retry.The test suite would benefit from an additional test case that verifies the behavior when an operation succeeds after an initial failure (retry mechanism if implemented).
fiftyone/core/odm/database.py (2)
324-352: Consider adding retry mechanism for transient errors.The function handles connection errors well, but could benefit from a retry mechanism for transient MongoDB errors.
+from tenacity import retry, stop_after_attempt, wait_exponential + +@retry( + stop=stop_after_attempt(3), + wait=wait_exponential(multiplier=1, min=4, max=10) +) def _get_fcv_and_version( client: pymongo.MongoClient, ) -> Tuple[Version, Version]:
413-457: Consider adding rollback capability.The FCV update is a critical operation. Consider adding rollback capability in case of partial failures.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/source/getting_started/install.rst(2 hunks)fiftyone/core/odm/database.py(4 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
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
- 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: build
🔇 Additional comments (2)
tests/unittests/fcv_tests.py (2)
1-14: LGTM! Well-organized imports.The imports are appropriately structured, including all necessary testing utilities and MongoDB error classes.
19-49: LGTM! Well-structured test for successful FCV update.The test effectively verifies both the command execution and warning message for a successful FCV update scenario.
@findtopher @kevin-dimichel can I get some eyes from you guys on this? |
|
Install Add a Dataset Verify MongoDB version: upgrade to this branch, fiftyone-db 0.4.0 verify MongoDB binary version: failure at list datasets: MongoDB looks correct and like it should be upgraded to 5... [ sorry, didn't mean to close this at all, must have been a strange keyboard combo ] |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
fiftyone/core/odm/database.py (1)
232-235:⚠️ Potential issueAdd error handling for FCV update failures.
While the FCV update integration point is correct, failures should be caught to prevent connection establishment failures.
- _update_fc_version(_client) + try: + _update_fc_version(_client) + except Exception as e: + logger.warning( + "Failed to update feature compatibility version: %s", str(e) + )
🧹 Nitpick comments (2)
fiftyone/core/odm/database.py (2)
324-352: Enhance error handling for MongoDB-related errors.The function should handle other MongoDB-related errors beyond connection issues.
try: current_version = client.admin.command( {"getParameter": 1, "featureCompatibilityVersion": 1} ) current_fcv = Version( current_version["featureCompatibilityVersion"]["version"] ) server_version = Version(client.server_info()["version"]) return current_fcv, server_version except ServerSelectionTimeoutError as e: raise ConnectionError("Could not connect to `mongod`") from e + except PyMongoError as e: + raise RuntimeError("Failed to retrieve MongoDB version information") from e
413-458: Enhance logging for better debugging.Add version information to the warning message for better debugging and auditing.
_logger.warning( - "Your MongoDB server version is newer than your feature " - "compatibility version. " - "Upgrading the feature compatibility version now." + "Your MongoDB server version (%s) is newer than your feature " + "compatibility version (%s). " + "Upgrading the feature compatibility version to %s now.", + server_version, fc_version, bumped )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fiftyone/core/odm/database.py(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: build / build
- GitHub Check: test / test-app
- GitHub Check: lint / eslint
- GitHub Check: e2e / test-e2e
- GitHub Check: build
🔇 Additional comments (1)
fiftyone/core/odm/database.py (1)
354-411: Add validation for version format.The implementation correctly handles FCV upgrade scenarios, but should validate version formats.
def _is_fcv_upgradeable(fc_version: Version, server_version: Version) -> bool: + def _is_valid_version(v: Version) -> bool: + return len(str(v).split('.')) >= 2 + + if not (_is_valid_version(fc_version) and _is_valid_version(server_version)): + _logger.error("Invalid version format detected") + return False + if (fc_version == foc.MIN_MONGODB_VERSION) and (
|
Seems to work great now: and |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unittests/fcv_tests.py (1)
28-242: Test coverage is thorough but could benefit from additional edge cases.The test suite covers essential scenarios including:
- Successful FCV updates
- Connection errors
- Version difference warnings
- Operation failures
Consider adding these test cases:
- Test for invalid version strings
- Test for non-numeric version components
- Test for empty version strings
def test_invalid_version_strings(self): test_cases = [ "invalid", "", "5.x", "5.", ] for invalid_version in test_cases: with self.subTest(invalid_version=invalid_version): with patch("pymongo.MongoClient") as mock_client: mock_admin = MagicMock() mock_client.admin = mock_admin mock_client.server_info.return_value = { "version": "5.0.0" } mock_admin.command.return_value = { "featureCompatibilityVersion": { "version": invalid_version } } # Should handle invalid versions gracefully _update_fc_version(mock_client)
📜 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(4 hunks)tests/unittests/fcv_tests.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: test / test-app
- GitHub Check: e2e / test-e2e
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: build / build
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: lint / eslint
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: build
🔇 Additional comments (6)
fiftyone/constants.py (1)
105-106: LGTM! Constants are well-defined for FCV management.The new constants are appropriately placed and their values align with MongoDB's versioning requirements:
MAX_ALLOWABLE_FCV_DELTA = 1ensures safe incremental upgradesMONGODB_SERVER_FCV_REQUIRED_CONFIRMATION = Version("7.0")correctly handles MongoDB 7.0+'s confirmation requirementtests/unittests/fcv_tests.py (1)
1-27: LGTM! Well-structured test setup with comprehensive imports.The test file is well-organized with:
- Appropriate imports for MongoDB errors and mocking
- Helper method for generating expected FCV update commands
fiftyone/core/odm/database.py (4)
232-235: Consider adding error handling for FCV updates during connection.The FCV update is correctly integrated into the connection establishment, but failures should be caught to prevent connection failures.
324-352: LGTM! Well-implemented FCV and version retrieval.The function correctly:
- Fetches both FCV and server version
- Handles connection errors
- Returns strongly typed results
354-411: Add input validation for version strings.While the function logic is sound, it should validate version string formats.
413-461: LGTM! Robust FCV update implementation with proper error handling.The function includes:
- Proper error handling for operation failures
- Conditional confirmation flag for MongoDB 7.0+
- Informative error messages
Let's verify the MongoDB version compatibility:
✅ Verification successful
MongoDB version compatibility implementation verified and consistent
The code correctly handles MongoDB version compatibility with proper constraints (min version 4.4), version delta checks, and special handling for MongoDB 7.0+ with the confirm flag. The implementation is well-tested and documented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check MongoDB version compatibility in the codebase # Test: Search for MongoDB version references to ensure consistency rg -A 2 "mongodb.*version|version.*mongodb" --ignore-caseLength of output: 7334
|
@benjaminpkane can you think of any other good battle-testing? I went through and performed this on a few VMs (from mongo versions 4.4 -> 5.4 -> 6.0.20 -> 7.0.4 -> 8.0.4) |
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.
How is this patch tested? If it is not, please explain why.
Unit tests were added to
tests/unittests/fcv_tests.py. The mongoDB instance is mocked in those tests so that the database is not corrupted during the other tests (and causes flakes).Manual tests were also performed.
Test 1: Nominal Upgrade Path On Debian and Ubuntu VM
$ sudo apt-get install -y python3-pip python3-venv git $ python3 -m venv .venv $ source .venv/bin/activate $ pip install fiftyoneIn a python interpreter:
Simulated a mongo standalone upgrade:
$ wget -O mongo-next.tgz \ https://fastdl.mongodb.org/linux/mongodb-linux-aarch64-ubuntu2204-7.0.4.tgz $ tar xvzf mongo-next.tgz $ find mongodb-linux-* \ -name mongod -exec cp {} \ $HOME/.venv/lib/python3.11/site-packages/fiftyone/db/bin/mongod \; $ $HOME/.venv/lib/python3.11/site-packages/fiftyone/db/bin/mongod --versionThen built fiftyone from source on my branch:
$ git clone https://github.com/voxel51/fiftyone.git \ -b afoley/AS-331-pymongo-fcv-updates $ cd fiftyone $ bash install.bash $ export PYTHONPATH=$PWD:$PYTHONPATHThen ran the app to verify the logs:
Ran the above again and also verified no repeated operations.
Finally, verified the FCV via mongosh:
Test 2: Ignore Upgrades When Using A Standalone DB
Asserted that these don't get executed on standalone (non-embedded)
databases.
Docs
Verified docs admonition (see below):
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
Documentation
Improvements
Testing