You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Addresses feedback from #440 to move the upload_artifacts_to_mlflow import from module level to function scope, avoiding potential circular import issues and making the dependency explicit.
Changes
Move from .mlflow_artifacts import upload_artifacts_to_mlflow from module-level (line 11) to inside upload_mlflow_artifacts() function where it's actually used
This makes the import conditional on the function being called rather than when the module is first imported, reducing import-time coupling.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
CopilotAI
changed the title
[WIP] Address feedback on MLflow artifact upload
Move MLflow import to function scope to avoid import-time dependencies
Dec 18, 2025
Yes, it's a valid pattern, but whether it's preferable depends on context:
Pros of moving import inside function (lazy import):
Avoids circular imports - If mlflow_artifacts.py ever imports from global_vars.py, module-level import would fail
Faster module load - mlflow_artifacts.py only loads when upload_mlflow_artifacts() is actually called
Conditional loading - Since MLflow upload only runs at end of training (and only if enabled), the import is truly deferred
Cons:
Against PEP 8 - Python style guide recommends imports at top of file
Slight runtime overhead - Import lookup happens each call (though Python caches, so negligible after first call)
Less visible dependencies - Harder to see module dependencies at a glance
Delayed error detection - Import errors surface at function call time, not module load time
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses feedback from #440 to move the
upload_artifacts_to_mlflowimport from module level to function scope, avoiding potential circular import issues and making the dependency explicit.Changes
from .mlflow_artifacts import upload_artifacts_to_mlflowfrom module-level (line 11) to insideupload_mlflow_artifacts()function where it's actually usedThis makes the import conditional on the function being called rather than when the module is first imported, reducing import-time coupling.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.