[cards] debugging statements.#3273
Conversation
Greptile SummaryThis PR adds a
Confidence Score: 4/5Safe to merge; changes are limited to debug logging paths that only activate when METAFLOW_DEBUG_CARD is set. All changes are confined to debug logging paths that only activate when METAFLOW_DEBUG_CARD is set, so normal card execution is unaffected. The dead-code ternary and unguarded UTF-8 decode in the exception handlers are worth cleaning up but cannot impact production runs. metaflow/plugins/cards/card_creator.py — the exception handler debug blocks have a dead-code conditional and an unguarded UTF-8 decode worth cleaning up. Important Files Changed
Reviews (1): Last reviewed commit: "[cards] debugging statements." | Re-trigger Greptile |
| except subprocess.CalledProcessError as e: | ||
| rep = e.output | ||
| fail = True | ||
| if debug.card and rep is not None: | ||
| debug.card_exec( | ||
| "render subprocess for %s %s output:\n%s" | ||
| % ( | ||
| card_uuid, | ||
| "FAILED" if fail else "succeeded", | ||
| rep.decode("utf-8"), | ||
| ) | ||
| ) | ||
| except subprocess.TimeoutExpired as e: | ||
| rep = e.output | ||
| fail = True | ||
| if debug.card and rep is not None: | ||
| debug.card_exec( | ||
| "render subprocess for %s %s output:\n%s" | ||
| % ( | ||
| card_uuid, | ||
| "FAILED" if fail else "succeeded", | ||
| rep.decode("utf-8"), | ||
| ) | ||
| ) |
There was a problem hiding this comment.
The
"FAILED" if fail else "succeeded" conditional is dead code in both exception handlers — fail is set to True on the line immediately above, so the else branch can never execute. The suggestion also adds errors="replace" to rep.decode("utf-8") to avoid a UnicodeDecodeError if subprocess output contains non-UTF-8 bytes.
| except subprocess.CalledProcessError as e: | |
| rep = e.output | |
| fail = True | |
| if debug.card and rep is not None: | |
| debug.card_exec( | |
| "render subprocess for %s %s output:\n%s" | |
| % ( | |
| card_uuid, | |
| "FAILED" if fail else "succeeded", | |
| rep.decode("utf-8"), | |
| ) | |
| ) | |
| except subprocess.TimeoutExpired as e: | |
| rep = e.output | |
| fail = True | |
| if debug.card and rep is not None: | |
| debug.card_exec( | |
| "render subprocess for %s %s output:\n%s" | |
| % ( | |
| card_uuid, | |
| "FAILED" if fail else "succeeded", | |
| rep.decode("utf-8"), | |
| ) | |
| ) | |
| except subprocess.CalledProcessError as e: | |
| rep = e.output | |
| fail = True | |
| if debug.card and rep is not None: | |
| debug.card_exec( | |
| "render subprocess for %s FAILED output:\n%s" | |
| % ( | |
| card_uuid, | |
| rep.decode("utf-8", errors="replace"), | |
| ) | |
| ) | |
| except subprocess.TimeoutExpired as e: | |
| rep = e.output | |
| fail = True | |
| if debug.card and rep is not None: | |
| debug.card_exec( | |
| "render subprocess for %s TIMED OUT output:\n%s" | |
| % ( | |
| card_uuid, | |
| rep.decode("utf-8", errors="replace"), | |
| ) | |
| ) |
| debug.card_exec( | ||
| "create card_uuid=%s mode=%s final=%s sync=%s runtime_card=%s save_metadata=%s" | ||
| % (card_uuid, mode, final, sync, runtime_card, save_metadata) | ||
| ) |
There was a problem hiding this comment.
Silent-ignore case not captured by debug log
When mode != "render" and not runtime_card the method returns early (line 82) before reaching this debug.card_exec call, so calls that are silently dropped are invisible even with debug enabled. Moving the debug statement before the if/elif/else block (or adding a separate log in the early-return branch) would make this case observable.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3273 +/- ##
=========================================
Coverage ? 28.94%
=========================================
Files ? 381
Lines ? 52525
Branches ? 9268
=========================================
Hits ? 15202
Misses ? 36287
Partials ? 1036 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
PR Type
Summary
Have a env var that allow debugging cards through metaflow's debug module.