fix(util): import MetaflowInternalError in read_artifacts_module - #3365
Open
Anai-Guo wants to merge 1 commit into
Open
fix(util): import MetaflowInternalError in read_artifacts_module#3365Anai-Guo wants to merge 1 commit into
Anai-Guo wants to merge 1 commit into
Conversation
`read_artifacts_module` raises `MetaflowInternalError` on both of its error paths, but never imports it, so every failure surfaces as `NameError: name 'MetaflowInternalError' is not defined` instead. The function is reached from `spin-step --artifacts-module`, so a bad path or a module without an `ARTIFACTS` variable hits it. Add the deferred import, matching `compress_list` in the same file (the import is function-local there to avoid a circular import with metaflow.exception).
Contributor
Greptile SummaryThis PR fixes
Confidence Score: 5/5The PR appears safe to merge and correctly restores the intended exception behavior. The new function-local import follows established patterns in the same module, avoids introducing a module-level circular dependency, and makes both existing error paths raise the documented exception type.
|
| Filename | Overview |
|---|---|
| metaflow/util.py | Adds the missing deferred import required by both existing MetaflowInternalError raise paths. |
Reviews (1): Last reviewed commit: "fix(util): import MetaflowInternalError ..." | Re-trigger Greptile
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Problem
metaflow/util.py::read_artifacts_moduleraisesMetaflowInternalErroron both of its error paths, and documents it in the docstring:but the name is never imported into
metaflow/util.py. So neither raise can construct the exception — the name lookup fails first and the caller gets:pyflakes metaflow/util.pyflags exactly these two lines (621, 626) as undefined names.The function is reachable from the CLI:
metaflow/cli_components/step_cmd.pyimports it andspin_stepcalls it whenever--artifacts-moduleis passed:so a mistyped path, or a module without an
ARTIFACTSvariable, produces aNameErrorinstead of the intended error.Fix
Add the deferred import, matching the existing idiom in this same file —
compress_list(line 352) does exactly this, function-locally, becausemetaflow.utilis imported early and a module-levelfrom metaflow.exception import ...would risk a circular import:2 lines added, nothing else touched.
Verification
Real upstream
metaflow/util.pyandmetaflow/exception.py, loaded under ametaflowpackage namespace with onlymetaflow._vendor.packaging.version(the single module-level metaflow import inutil.py) andmetaflow.extension_support(exception.py's extension hook, off the path under test) stubbed:NameError: name 'MetaflowInternalError' is not definedMetaflowInternalError: Error reading file ...ARTIFACTSNameError: name 'MetaflowInternalError' is not definedMetaflowInternalError: Error reading file ...ARTIFACTS = {'a': 1}{'a': 1}{'a': 1}(unchanged)pyflakes metaflow/util.pyafter the change: no undefined names.Note (not changed here)
The explicit
raise MetaflowInternalError("Module ... does not contain ARTIFACTS variable")inside thetryis caught by the function's own broadexcept Exception as eand re-wrapped as"Error reading file ...", so that more specific message never reaches the user. That is pre-existing and independent of this fix; happy to add anexcept MetaflowInternalError: raisepassthrough in a follow-up if you'd like it preserved.🤖 Generated with Claude Code