Migrate core MLflow integration from file store to SQLite#8894
Migrate core MLflow integration from file store to SQLite#8894Lawson-Darrow wants to merge 5 commits into
Conversation
MLflow 3.13+ raises for the filesystem (file store) tracking backend unless MLFLOW_ALLOW_FILE_STORE=true is set. Migrate MONAI core MLflow usage off the file store to the recommended SQLite backend. - MLFlowHandler defaults to sqlite:///<cwd>/mlruns.db when no tracking_uri is given and MLFLOW_TRACKING_URI is unset. For any local SQLite tracking_uri it derives an mlruns artifact directory next to the database file, mirroring the old layout. Remote and other backends are left to MLflow to decide. - Add an optional artifact_location argument to MLFlowHandler to override the derived location. Any explicit tracking_uri or env var is passed through. - DEFAULT_MLFLOW_SETTINGS uses a sqlite tracking_uri under <output_dir>/mlruns.db. - Add monai.utils.path_to_sqlite_uri to build a cross-platform SQLite URI. - Migrate the bundle-run and federated-learning tests off file-store tracking URIs and clean up the derived artifact directory. The model zoo bundles that pass an explicit file URI are left for a separate PR. Closes Project-MONAI#8891. Signed-off-by: Lawson Darrow <lawson.darrow@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR replaces filesystem MLflow defaults with SQLite-backed tracking URIs (sqlite:///.../mlruns.db), adds monai.utils.path_to_sqlite_uri and re-exports it, updates bundle docs/defaults to the new DB/artifact layout, extends MLFlowHandler with an artifact_location argument, derives artifact_location for local SQLite DBs and passes it to mlflow.create_experiment, adds defensive disposal of MLflow SQLite engines on close, and updates integration and unit tests to use SQLite URIs and adjusted cleanup/assertions. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/fl/monai_algo/test_fl_monai_algo.py (1)
205-234: ⚡ Quick winCleanup is inline, so a failed assertion pollutes the repo's
testing_data.The
.db/mlrunsartifacts are written into_data_dir(a tracked dir). Cleanup only runs after theassertTrue/assertGreaterchecks, so any earlier failure leaves SQLite files in the working tree.Register cleanup up-front via
self.addCleanup(...)so it runs even on failure. Note SQLite may emit*.db-wal/*.db-shmsidecars; themlflow_*glob intest_evaluatecovers them, but the explicitos.remove(mlflow_override.db)intest_train(Line 206) does not.🤖 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 `@tests/fl/monai_algo/test_fl_monai_algo.py` around lines 205 - 234, The tests clean up mlflow artifacts inline which can leave files in _data_dir on assertion failures; change both test_train and test_evaluate to register cleanup actions up-front using self.addCleanup so removal always runs, and in those cleanups remove all mlflow-related files using glob patterns (e.g., "mlflow_*", "mlflow_override.db", plus sidecars "*.db-wal" and "*.db-shm") and remove the "mlruns" directory if present; locate cleanup registration near the start of test_evaluate and test_train (references: test_evaluate, test_train, _data_dir) and replace explicit os.remove/os.path.isdir removal at the end with these guaranteed addCleanup handlers.monai/utils/misc.py (1)
731-743: ⚡ Quick winComplete the public helper docstring.
This new exported helper omits a Google-style
Returns:section. Please document the exact URI format returned here, since the slash-count examples are part of the contract. As per coding guidelines, "Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings."Possible fix
def path_to_sqlite_uri(path: PathLike) -> str: """ Convert a database file path to a SQLite connection URI, e.g. for use as an MLflow ``tracking_uri``. If not an absolute path, it is converted to an absolute path first. @@ Args: path: input database file path, can be a string or `Path` object. + + Returns: + A SQLite connection URI using an absolute POSIX-form path. """ return f"sqlite:///{Path(path).absolute().as_posix()}"🤖 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 `@monai/utils/misc.py` around lines 731 - 743, The docstring for path_to_sqlite_uri is missing a Google-style Returns: section and exception notes; update the public helper's docstring to include a Returns: that precisely states the returned URI string format (e.g., on Windows "sqlite:///C:/path/db.sqlite" and on POSIX "sqlite:////path/db.sqlite"), explain that the function returns an absolute-path SQLite URI using POSIX forward slashes, and add a Raises: section listing any obvious exceptions (e.g., TypeError if path is an invalid type or any exceptions propagated by Path(path).absolute()); keep this documentation adjacent to the existing description in the path_to_sqlite_uri function.
🤖 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.
Inline comments:
In `@monai/handlers/mlflow_handler.py`:
- Around line 187-197: The artifact_location derivation only runs when the local
tracking_uri arg is set; update the logic in mlflow_handler (around the
tracking_uri handling and the artifact_location assignment) to consider the
effective tracking URI coming from either the tracking_uri parameter or
os.environ["MLFLOW_TRACKING_URI"]: compute a single effective_tracking_uri
(e.g., tracking_uri or env var) and then, if self.artifact_location is None and
effective_tracking_uri startswith "sqlite:///", derive db_path via
Path(effective_tracking_uri[len("sqlite:///"):]) and set self.artifact_location
= path_to_uri(db_path.parent / "mlruns") using the existing
path_to_sqlite_uri/path_to_uri helpers so both explicit arg and env-provided
SQLite URIs result in the same sibling mlruns artifact location.
---
Nitpick comments:
In `@monai/utils/misc.py`:
- Around line 731-743: The docstring for path_to_sqlite_uri is missing a
Google-style Returns: section and exception notes; update the public helper's
docstring to include a Returns: that precisely states the returned URI string
format (e.g., on Windows "sqlite:///C:/path/db.sqlite" and on POSIX
"sqlite:////path/db.sqlite"), explain that the function returns an absolute-path
SQLite URI using POSIX forward slashes, and add a Raises: section listing any
obvious exceptions (e.g., TypeError if path is an invalid type or any exceptions
propagated by Path(path).absolute()); keep this documentation adjacent to the
existing description in the path_to_sqlite_uri function.
In `@tests/fl/monai_algo/test_fl_monai_algo.py`:
- Around line 205-234: The tests clean up mlflow artifacts inline which can
leave files in _data_dir on assertion failures; change both test_train and
test_evaluate to register cleanup actions up-front using self.addCleanup so
removal always runs, and in those cleanups remove all mlflow-related files using
glob patterns (e.g., "mlflow_*", "mlflow_override.db", plus sidecars "*.db-wal"
and "*.db-shm") and remove the "mlruns" directory if present; locate cleanup
registration near the start of test_evaluate and test_train (references:
test_evaluate, test_train, _data_dir) and replace explicit
os.remove/os.path.isdir removal at the end with these guaranteed addCleanup
handlers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8395b304-ef2f-4c4e-8469-2ef1368131be
📒 Files selected for processing (8)
monai/bundle/scripts.pymonai/bundle/utils.pymonai/handlers/mlflow_handler.pymonai/utils/__init__.pymonai/utils/misc.pytests/fl/monai_algo/test_fl_monai_algo.pytests/handlers/test_handler_mlflow.pytests/integration/test_integration_bundle_run.py
| # Only fall back to the SQLite default when the caller gave no tracking_uri and the | ||
| # `MLFLOW_TRACKING_URI` environment variable is unset, so that env-var configuration keeps | ||
| # working. When it is set, `tracking_uri` stays None and MLflow resolves the env var. | ||
| if not tracking_uri and not os.environ.get("MLFLOW_TRACKING_URI"): | ||
| tracking_uri = path_to_sqlite_uri(os.path.join(os.getcwd(), "mlruns.db")) | ||
| # For a local SQLite backend, keep run artifacts in an `mlruns` directory next to the | ||
| # database file (mirroring the previous file-store layout) unless the caller set | ||
| # `artifact_location`. Other backends (e.g. a remote server) are left to MLflow to decide. | ||
| if self.artifact_location is None and tracking_uri and tracking_uri.startswith("sqlite:///"): | ||
| db_path = Path(tracking_uri[len("sqlite:///") :]) | ||
| self.artifact_location = path_to_uri(db_path.parent / "mlruns") |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In MLflow 3.x, if MLFLOW_TRACKING_URIpoints to a local SQLite database andcreate_experiment()is called withoutartifact_location, what default artifact location is used?
💡 Result:
In MLflow 3.x, if MLFLOW_TRACKING_URI points to a local SQLite database and create_experiment() is called without an explicit artifact_location, the default artifact location is a path inside the local file system, typically the ./mlruns directory [1][2][3]. When using a local SQLite backend, MLflow relies on the local file system for artifact storage unless a specific --default-artifact-root is configured on the tracking server [2][4]. If no artifact_location is provided during the creation of an experiment, MLflow defaults to a local path [1][3]. It is important to note that this local path is relative to the directory where the code is executing [1][2]. In production or multi-node environments, this default can lead to issues because the client and server may resolve the path to different physical locations on their respective disks [1][3]. To avoid this, it is recommended to explicitly set an artifact_location (such as an S3 bucket or other remote storage) when creating an experiment, or to configure the --default-artifact-root when starting the MLflow tracking server [1][2][5][3].
Citations:
- 1: https://mlflow.org/docs/3.5.0/self-hosting/architecture/artifact-store/
- 2: [BUG] mlflow server doesnot start without default-artifact-root argument mlflow/mlflow#7139
- 3: https://mlflow.org/docs/latest/self-hosting/architecture/artifact-store/
- 4: https://mlflow.org/docs/2.0.0/tracking.html
- 5: https://stackoverflow.com/questions/70084661/setup-mlflow-backend-sqlite-and-artifact-azure-blob-storage-stores
Derive artifact_location consistently for both arg and MLFLOW_TRACKING_URI-provided SQLite
- Current code derives sibling
mlrunsonly whentracking_uriis explicitly passed; whenMLFLOW_TRACKING_URI=sqlite:///...,tracking_uristaysNone, soartifact_locationremains unset and MLflow uses its default local artifact root (typically./mlruns, relative to the process CWD). This causes different artifact placement for equivalent local SQLite setups (config split-brain).
Possible fix
self.artifact_location = artifact_location
+ effective_tracking_uri = tracking_uri or os.environ.get("MLFLOW_TRACKING_URI")
# Only fall back to the SQLite default when the caller gave no tracking_uri and the
# `MLFLOW_TRACKING_URI` environment variable is unset, so that env-var configuration keeps
# working. When it is set, `tracking_uri` stays None and MLflow resolves the env var.
- if not tracking_uri and not os.environ.get("MLFLOW_TRACKING_URI"):
- tracking_uri = path_to_sqlite_uri(os.path.join(os.getcwd(), "mlruns.db"))
+ if not effective_tracking_uri:
+ tracking_uri = effective_tracking_uri = path_to_sqlite_uri(os.path.join(os.getcwd(), "mlruns.db"))
# For a local SQLite backend, keep run artifacts in an `mlruns` directory next to the
# database file (mirroring the previous file-store layout) unless the caller set
# `artifact_location`. Other backends (e.g. a remote server) are left to MLflow to decide.
- if self.artifact_location is None and tracking_uri and tracking_uri.startswith("sqlite:///"):
- db_path = Path(tracking_uri[len("sqlite:///") :])
+ if self.artifact_location is None and effective_tracking_uri and effective_tracking_uri.startswith("sqlite:///"):
+ db_path = Path(effective_tracking_uri[len("sqlite:///") :])
self.artifact_location = path_to_uri(db_path.parent / "mlruns")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Only fall back to the SQLite default when the caller gave no tracking_uri and the | |
| # `MLFLOW_TRACKING_URI` environment variable is unset, so that env-var configuration keeps | |
| # working. When it is set, `tracking_uri` stays None and MLflow resolves the env var. | |
| if not tracking_uri and not os.environ.get("MLFLOW_TRACKING_URI"): | |
| tracking_uri = path_to_sqlite_uri(os.path.join(os.getcwd(), "mlruns.db")) | |
| # For a local SQLite backend, keep run artifacts in an `mlruns` directory next to the | |
| # database file (mirroring the previous file-store layout) unless the caller set | |
| # `artifact_location`. Other backends (e.g. a remote server) are left to MLflow to decide. | |
| if self.artifact_location is None and tracking_uri and tracking_uri.startswith("sqlite:///"): | |
| db_path = Path(tracking_uri[len("sqlite:///") :]) | |
| self.artifact_location = path_to_uri(db_path.parent / "mlruns") | |
| effective_tracking_uri = tracking_uri or os.environ.get("MLFLOW_TRACKING_URI") | |
| # Only fall back to the SQLite default when the caller gave no tracking_uri and the | |
| # `MLFLOW_TRACKING_URI` environment variable is unset, so that env-var configuration keeps | |
| # working. When it is set, `tracking_uri` stays None and MLflow resolves the env var. | |
| if not effective_tracking_uri: | |
| tracking_uri = effective_tracking_uri = path_to_sqlite_uri(os.path.join(os.getcwd(), "mlruns.db")) | |
| # For a local SQLite backend, keep run artifacts in an `mlruns` directory next to the | |
| # database file (mirroring the previous file-store layout) unless the caller set | |
| # `artifact_location`. Other backends (e.g. a remote server) are left to MLflow to decide. | |
| if self.artifact_location is None and effective_tracking_uri and effective_tracking_uri.startswith("sqlite:///"): | |
| db_path = Path(effective_tracking_uri[len("sqlite:///") :]) | |
| self.artifact_location = path_to_uri(db_path.parent / "mlruns") |
🤖 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 `@monai/handlers/mlflow_handler.py` around lines 187 - 197, The
artifact_location derivation only runs when the local tracking_uri arg is set;
update the logic in mlflow_handler (around the tracking_uri handling and the
artifact_location assignment) to consider the effective tracking URI coming from
either the tracking_uri parameter or os.environ["MLFLOW_TRACKING_URI"]: compute
a single effective_tracking_uri (e.g., tracking_uri or env var) and then, if
self.artifact_location is None and effective_tracking_uri startswith
"sqlite:///", derive db_path via
Path(effective_tracking_uri[len("sqlite:///"):]) and set self.artifact_location
= path_to_uri(db_path.parent / "mlruns") using the existing
path_to_sqlite_uri/path_to_uri helpers so both explicit arg and env-provided
SQLite URIs result in the same sibling mlruns artifact location.
With close_on_complete=True the handler closes the run on the engine COMPLETED event, which sets cur_run to None, so reading handler.cur_run.info afterwards raised AttributeError. Use close_on_complete=False and close explicitly after the metric check, matching test_metrics_track. Signed-off-by: Lawson Darrow <lawson.darrow@gmail.com>
On Windows the open SQLite connection kept the database file locked, so deleting it during test cleanup raised PermissionError (WinError 32). close() now disposes MLflow's SQLAlchemy engine for local SQLite tracking backends (no-op otherwise), which is also a genuine resource-cleanup improvement. The construct-only handler tests now call close() in a finally so their temp databases can be removed. Signed-off-by: Lawson Darrow <lawson.darrow@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
monai/handlers/mlflow_handler.py (1)
186-197:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle env-provided local SQLite URIs the same way.
If
MLFLOW_TRACKING_URIis set tosqlite:///..., Line 190 correctly skips the fallback, but Lines 195-197 also skip siblingmlrunsderivation because they only inspect thetracking_uriarg.create_experiment()then getsartifact_location=None, so env-based local SQLite uses MLflow’s default artifact root instead of<db dir>/mlruns. A smalleffective_tracking_uri = tracking_uri or os.environ.get("MLFLOW_TRACKING_URI")fix here would keep the two config paths consistent and is worth a unit test too.🤖 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 `@monai/handlers/mlflow_handler.py` around lines 186 - 197, The artifact-location derivation skips env-provided local SQLite URIs because the code only checks the local argument tracking_uri; compute an effective_tracking_uri = tracking_uri or os.environ.get("MLFLOW_TRACKING_URI") (after the existing fallback logic that may set tracking_uri via path_to_sqlite_uri) and use effective_tracking_uri for the sqlite:/// startswith check and to derive db_path/artifact_location (referencing self.artifact_location, tracking_uri, MLFLOW_TRACKING_URI, and path_to_sqlite_uri); this ensures env-set sqlite URIs produce a sibling `mlruns` artifact folder like the explicit tracking_uri path would.
🧹 Nitpick comments (1)
monai/handlers/mlflow_handler.py (1)
383-419: ⚡ Quick winComplete the new docstrings.
_dispose_sqlite_store()andclose()need Google-styleReturns/Raisessections. Both methods now have non-trivial side effects and can propagate MLflow/engine disposal failures, so the contract should be explicit. As per coding guidelines, "Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings."🤖 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 `@monai/handlers/mlflow_handler.py` around lines 383 - 419, Add Google-style Returns and Raises sections to the docstrings of _dispose_sqlite_store and close: for _dispose_sqlite_store describe that it returns None and may raise exceptions from MLflow internals or SQLAlchemy engine disposal (e.g., AttributeError, RuntimeError, sqlalchemy.exc.SQLAlchemyError) when reaching into client internals or calling engine.dispose(); for close describe it returns None, may raise MLflow exceptions when calling set_terminated or any exceptions from _dispose_sqlite_store (document that exceptions from MLflow client or engine disposal can propagate), and note side effects (terminates current run and releases local SQLite resources). Ensure the docstrings for both include a short Description, a Returns: "None", and a Raises: listing the possible exception types and the conditions under which they are raised, referencing the methods _dispose_sqlite_store and client.set_terminated in the text.
🤖 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.
Duplicate comments:
In `@monai/handlers/mlflow_handler.py`:
- Around line 186-197: The artifact-location derivation skips env-provided local
SQLite URIs because the code only checks the local argument tracking_uri;
compute an effective_tracking_uri = tracking_uri or
os.environ.get("MLFLOW_TRACKING_URI") (after the existing fallback logic that
may set tracking_uri via path_to_sqlite_uri) and use effective_tracking_uri for
the sqlite:/// startswith check and to derive db_path/artifact_location
(referencing self.artifact_location, tracking_uri, MLFLOW_TRACKING_URI, and
path_to_sqlite_uri); this ensures env-set sqlite URIs produce a sibling `mlruns`
artifact folder like the explicit tracking_uri path would.
---
Nitpick comments:
In `@monai/handlers/mlflow_handler.py`:
- Around line 383-419: Add Google-style Returns and Raises sections to the
docstrings of _dispose_sqlite_store and close: for _dispose_sqlite_store
describe that it returns None and may raise exceptions from MLflow internals or
SQLAlchemy engine disposal (e.g., AttributeError, RuntimeError,
sqlalchemy.exc.SQLAlchemyError) when reaching into client internals or calling
engine.dispose(); for close describe it returns None, may raise MLflow
exceptions when calling set_terminated or any exceptions from
_dispose_sqlite_store (document that exceptions from MLflow client or engine
disposal can propagate), and note side effects (terminates current run and
releases local SQLite resources). Ensure the docstrings for both include a short
Description, a Returns: "None", and a Raises: listing the possible exception
types and the conditions under which they are raised, referencing the methods
_dispose_sqlite_store and client.set_terminated in the text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 66636e0e-e9f1-4f28-920b-cb8b8aa3c127
📒 Files selected for processing (2)
monai/handlers/mlflow_handler.pytests/handlers/test_handler_mlflow.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/handlers/test_handler_mlflow.py
The MLflow file-store to SQLite migration leaves an open SQLAlchemy SQLite connection, which on Windows locks the .db file. The FL workflow can finish without closing the tracking handler that holds mlflow_1.db, so test_fl_monai_algo failed on full-dep (windows-latest) when os.remove'ing the mlflow_*.db files. Dispose the test's mlflow*.db SQLite engines before removing the files. Signed-off-by: Lawson Darrow <lawson.darrow@gmail.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/fl/monai_algo/test_fl_monai_algo.py`:
- Around line 182-209: The broad except in _dispose_sqlite_engines() hides
unexpected errors when probing SQLAlchemy Engine.url; narrow the failure mode by
checking attributes or catching AttributeError only: verify hasattr(url,
"get_backend_name") and hasattr(url, "database") before calling
url.get_backend_name() and accessing url.database (or replace the broad except
Exception with except AttributeError) so only missing-attribute issues are
swallowed while other errors surface; reference Engine, url.get_backend_name,
and url.database in your change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 264a7262-eca1-4fdf-acdd-f5f950eebd2d
📒 Files selected for processing (1)
tests/fl/monai_algo/test_fl_monai_algo.py
| def _dispose_sqlite_engines(): | ||
| """Dispose MLflow's open SQLAlchemy SQLite engines so the test ``.db`` files can be removed. | ||
|
|
||
| MLflow keeps a SQLite connection open for the lifetime of its client; on Windows that | ||
| locks the database file and breaks cleanup. ``MLFlowHandler.close()`` releases it, but a | ||
| workflow may finish without closing every handler, so dispose defensively here before | ||
| deleting the files. Scoped to the test's ``mlflow*.db`` backends so unrelated (e.g. | ||
| in-memory) sqlite engines elsewhere in the process are left untouched. | ||
| """ | ||
| import gc | ||
|
|
||
| try: | ||
| from sqlalchemy.engine import Engine | ||
| except ImportError: | ||
| return | ||
| gc.collect() | ||
| for obj in gc.get_objects(): | ||
| if not isinstance(obj, Engine): | ||
| continue | ||
| try: | ||
| url = obj.url | ||
| db = url.database if url.get_backend_name() == "sqlite" else None | ||
| # the test backends are all files named ``mlflow*.db``; match those only so | ||
| # unrelated (e.g. in-memory) sqlite engines in the process are left untouched. | ||
| if db and os.path.basename(db).startswith("mlflow"): | ||
| obj.dispose() | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm SQLAlchemy is pinned/available and inspect the URL API.
fd -t f 'requirements.*\.txt|setup\.cfg|pyproject\.toml' | xargs rg -n -i 'sqlalchemy'Repository: Project-MONAI/MONAI
Length of output: 47
🌐 Web query:
Does SQLAlchemy URL expose get_backend_name() and a database attribute in 1.4 and 2.0?
💡 Result:
Yes, the SQLAlchemy URL object exposes both the get_backend_name method and the database attribute in both SQLAlchemy 1.4 and 2.0 [1][2]. Both were formalized as part of the URL object's overhaul in version 1.4, where the URL object was redesigned to be an immutable namedtuple-like structure [3][4]. The get_backend_name method returns the database backend name (the portion of the drivername to the left of the plus sign) [1][2], and the database attribute provides access to the database name specified in the URL [1][2].
Citations:
- 1: https://docs.sqlalchemy.org/14/core/engines.html
- 2: https://docs.sqlalchemy.org/core/engines.html
- 3: https://docs.sqlalchemy.org/en/20/changelog/migration_14.html
- 4: https://www.sqlalchemy.org/docs/14/changelog/migration_14.html
🏁 Script executed:
#!/bin/bash
# Find where SQLAlchemy is declared/pinned in this repo.
rg -n --hidden --no-ignore-vcs -S "sqlalchemy" pyproject.toml setup.cfg setup.py requirements*.txt */requirements*.txt 2>/dev/null || trueRepository: Project-MONAI/MONAI
Length of output: 45
🌐 Web query:
sqlalchemy version pins MONAI supported versions (sqlalchemy) fl monai_algo?
💡 Result:
MONAI does not appear to pin SQLAlchemy versions. What I found in MONAI packaging/dependency metadata: - MONAI’s declared install requirements in setup.cfg list torch and numpy, plus many optional dependencies under extras; SQLAlchemy is not present in the shown install_requires or extras list [1][2]. - The MONAI PyPI page describes dependency versioning for major releases and points to requirements*.txt files, but the dependency table excerpt shown there does not include SQLAlchemy [3]. So there is no explicit “sqlalchemy version pin” I can cite from MONAI’s published dependency metadata for the monai/fl client algorithm component (monai_algo.py) either—monai_algo.py itself is code for FL client behavior and does not specify SQLAlchemy dependencies [4]. If you meant “MONAI FL (monai_algo) indirectly depends on SQLAlchemy via another library”, tell me which MONAI version you’re using (e.g., 1.3.0/1.4.0/1.5.x) and your install extras (e.g., monai[all]) and I can trace the transitive dependency chain from that exact lockfile/requirements output.
Citations:
- 1: https://github.com/Project-MONAI/MONAI/blob/cafc1feb5063ccbc206a71d2f006b3ea386a74b4/setup.cfg
- 2: https://github.com/Project-MONAI/MONAI/blob/daf9f054303d3d9a1ff7881f43b09bc82a51cf24/setup.cfg
- 3: https://pypi.org/project/monai/
- 4: https://github.com/Project-MONAI/MONAI/blob/dev/monai/fl/client/monai%5Falgo.py
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List likely dependency declaration files
fd -t f -i 'requirements*.txt|setup.cfg|pyproject.toml|setup.py' . || true
# Search for sqlalchemy/sqlalchemy in those files
for f in $(fd -t f -i 'requirements*.txt|setup.cfg|pyproject.toml|setup.py' .); do
rg -n -i 'sqlalchemy' "$f" || true
doneRepository: Project-MONAI/MONAI
Length of output: 146
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm there aren't other dependency declaration files referencing sqlalchemy.
rg -n -i --hidden --no-ignore-vcs "sqlalchemy" \
-g 'requirements*.txt' -g 'setup.cfg' -g 'pyproject.toml' -g 'setup.py' -g 'Pipfile*' -g '*.yml' -g '*.yaml' \
. | head -n 50Repository: Project-MONAI/MONAI
Length of output: 45
Narrow the failure mode of _dispose_sqlite_engines() (SQLAlchemy URL API)
- SQLAlchemy’s URL object includes both
get_backend_name()anddatabasein the 1.4/2.0 API, so the current attribute access matches upstream. - MONAI repo doesn’t pin SQLAlchemy in the searched dependency metadata; to avoid silently neutering cleanup on any transitive/version drift, narrow the
except Exception:to expected errors (e.g.,AttributeError) or gate withhasattr(url, "get_backend_name")/hasattr(url, "database")so failures aren’t fully swallowed.
🧰 Tools
🪛 Ruff (0.15.15)
[error] 208-209: try-except-pass detected, consider logging the exception
(S110)
[warning] 208-208: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@tests/fl/monai_algo/test_fl_monai_algo.py` around lines 182 - 209, The broad
except in _dispose_sqlite_engines() hides unexpected errors when probing
SQLAlchemy Engine.url; narrow the failure mode by checking attributes or
catching AttributeError only: verify hasattr(url, "get_backend_name") and
hasattr(url, "database") before calling url.get_backend_name() and accessing
url.database (or replace the broad except Exception with except AttributeError)
so only missing-attribute issues are swallowed while other errors surface;
reference Engine, url.get_backend_name, and url.database in your change.
gc.get_objects() can include dead weakref proxies, whose isinstance() raises ReferenceError. The check sat outside the try/except, so it escaped and errored the FL tests on Linux (full-dep ubuntu-latest). Move it inside the guard. Signed-off-by: Lawson Darrow <lawson.darrow@gmail.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Closes #8891. Implements Option 2 from the issue discussion.
MLflow 3.13+ raises for the filesystem (file store) tracking backend unless
MLFLOW_ALLOW_FILE_STORE=trueis set. This migrates MONAI core MLflow usage off the file store to the recommended SQLite backend.MLFlowHandlerdefaults tosqlite:///<cwd>/mlruns.dbwhen notracking_uriis given andMLFLOW_TRACKING_URIis unset. For any local SQLitetracking_uriit derives anmlrunsartifact directory next to the database file, mirroring the previous layout. Remote and other backends are left to MLflow to decide.artifact_locationargument toMLFlowHandlerto override the derived location. Any explicittracking_uri(including afile://URI) or env var is passed through unchanged.DEFAULT_MLFLOW_SETTINGSnow uses a SQLitetracking_uriunder<output_dir>/mlruns.db, keeping artifacts under<output_dir>/mlruns.monai.utils.path_to_sqlite_urito build a cross-platform SQLite URI.The model zoo bundles that pass an explicit file URI (e.g. vista3d) are left for a separate PR as discussed in the issue.
Unit tests cover the SQLite default, explicit
tracking_uripassthrough, explicit and derivedartifact_location, remotetracking_uri(no local artifact root injected), and a basic run flow.