Skip to content

Conversation

Copy link

Copilot AI commented Dec 18, 2025

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.

Copilot AI changed the title [WIP] Address feedback on MLflow artifact upload Move MLflow import to function scope to avoid import-time dependencies Dec 18, 2025
Copilot AI requested a review from gphuang December 18, 2025 10:22
@gphuang gphuang closed this Dec 18, 2025
@gphuang
Copy link

gphuang commented 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

@gphuang gphuang deleted the copilot/sub-pr-440 branch December 18, 2025 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants