Conversation
There was a problem hiding this comment.
Thanks — the logging half of this is fine, but the validation block has a logic bug and a regression.
1. core/data_manager.py:54-59 — the len(df) == 0 branch is unreachable, and header-only uploads are now rejected.
DataFrame.empty is True when any axis has length 0, so a frame with columns but zero rows already satisfies df.empty at line 54 and raises. That makes the warning at lines 58-59 dead code, and the two blocks contradict each other: line 56 rejects the zero-row case while lines 58-59 clearly intend to accept it with a warning.
It is also a behavior change for a supported workflow. A CSV/paste with headers and no data rows used to load fine and get padded by the very next block (# Pad with empty rows so user can add more data, lines 63-68), letting the user type values into the editor. After this change that upload fails outright. If the intent is only to catch genuinely malformed input, the guard should key on columns:
if len(df.columns) == 0:
logger.warning("Malformed upload: shape=%s", df.shape)
raise ValueError("Uploaded data is empty or has no columns.")
if len(df) == 0:
logger.warning("No data rows in upload, only columns: %s", list(df.columns))If rejecting zero-row uploads is intended, then lines 58-59 should be deleted and the PR description should say so — but please confirm that deliberately, since it undoes the padding feature.
2. core/data_manager.py:56 — the new error message never reaches the user.
All three callers (load_csv lines 20-21, load_excel lines 30-31, load_from_paste lines 47-48) catch bare Exception and return a canned "Unable to load CSV file. Please check the format.". So the ValueError("Uploaded data is empty or has no columns.") is swallowed and an empty upload surfaces as a misleading format error. To get the diagnostic benefit the PR is after, the loaders need to let the specific message through, e.g.:
except ValueError as e:
return False, str(e)
except Exception:
return False, "Unable to load CSV file. Please check the format."Minor
- No test covers the new validation.
tests/test_data_manager.pyalready has aload_csvtest to extend — a case for zero columns and one for header-only would pin down the intended behavior. - Trailing whitespace on the blank lines 57, 60, and 62.
Scope is otherwise fine and the _auto_detect_type debug lines look correct (line 102 cannot divide by zero given the len(series) == 0 early return).
…ion errors _apply_loaded_df rejected any zero-row DataFrame via df.empty, since DataFrame.empty is True whenever any axis has length 0 - this made the "only columns" warning branch dead code and broke header-only CSV/paste uploads that used to load fine and get padded for editing. Now only frames with zero columns are rejected. Also, the new ValueError from that validation was being caught by the callers' bare `except Exception` and replaced with a generic format error, so it never reached the user. load_csv/load_excel/load_from_paste now catch ValueError separately and return its message. Added regression tests for header-only uploads (still load and pad) and for the no-columns case (rejected, error message not swallowed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
b9fca11 to
75e3092
Compare
|
Both points in the review checked out against the code, fixed both: 1. Header-only uploads were being rejected (confirmed bug). 2. The new Minor: added the missing test coverage and cleaned up the trailing whitespace, both as suggested. New regression tests in
Checks: Nothing to push back on — both findings were accurate and reproducible. |
There was a problem hiding this comment.
Thanks — the core validation in _apply_loaded_df is sound (a zero-column frame, which pd.read_excel really can return for a blank sheet, is rejected before set_df, and the header-only case still pads to 20 editable rows). Two issues with the error-surfacing half of the change:
1. except ValueError is too broad — it leaks raw pandas parser text to end users (core/data_manager.py:20, :32, :51)
The intent (per the commit message) is to let the one validation message from core/data_manager.py:62 reach the caller. But pandas.errors.ParserError and pandas.errors.EmptyDataError are both ValueError subclasses, and pd.read_excel raises plain ValueError for engine/sheet problems. So these handlers now catch far more than the validation error, and pages/data_input.py:34 renders it verbatim:
- Ragged CSV →
Error loading file: Error tokenizing data. C error: Expected 3 fields in line 4, saw 5 - Empty file →
Error loading file: No columns to parse from file - Bad workbook →
Error loading file: Excel file format cannot be determined, you must specify an engine manually.
That defeats the deliberate generic messages the except Exception branches were there to provide, and it is an unstated behavior change for every malformed upload — not just empty ones.
Suggested fix: define a dedicated exception and catch only that, leaving pandas errors on the generic path.
class DataValidationError(ValueError):
"""Raised when a loaded frame fails our own validation."""Raise it at :62 and use except DataValidationError as e: return False, str(e) in all three loaders.
2. test_load_csv_with_no_columns_reports_specific_error_not_generic_one (tests/test_data_manager.py:94) does not test what its comment says
load_csv(io.StringIO("")) raises EmptyDataError inside pd.read_csv, so _apply_loaded_df is never reached. The test passes, but only because it is asserting on pandas' message, not on the ValueError raised at :62. With fix #1 applied it would fail. Please drive the validation path through a loader that actually reaches _apply_loaded_df (e.g. load_excel with a blank sheet, or monkeypatching pd.read_csv to return pd.DataFrame()) and assert on the exact expected message rather than != the generic one.
Minor, non-blocking: logger.warning at :64 logs user column names at warning level, and there is no logging configuration anywhere in the app, so the new logger.debug calls are inert by default — worth confirming that is intended.
|
Auto-fix budget exhausted after 1 rounds — not attempting another pass. Two bots disagreeing past this point usually means the proposal needs a human call, or is not worth the churn. Leaving the PR as-is; |
What
Add minimum data dimension validation to catch empty/malformed uploads and debug logging to auto-detection decisions in _apply_loaded_df.
Why
Helps catch and troubleshoot empty uploads and provides visibility into variable type detection for debugging.