Skip to content

Add data validation and auto-detection debug logging - #5

Open
ssevera1 wants to merge 2 commits into
mainfrom
improve/20260918-141632
Open

ssevera1 wants to merge 2 commits into
mainfrom
improve/20260918-141632

Conversation

@ssevera1

Copy link
Copy Markdown
Owner

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.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.py already has a load_csv test 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>
@ssevera1
ssevera1 force-pushed the improve/20260918-141632 branch from b9fca11 to 75e3092 Compare September 18, 2026 14:21
@claude

claude Bot commented Sep 18, 2026

Copy link
Copy Markdown

Both points in the review checked out against the code, fixed both:

1. Header-only uploads were being rejected (confirmed bug). DataFrame.empty is True whenever either axis has length 0, so a header-only frame (columns, zero rows) already tripped the df.empty check at the old line 54 — the len(df) == 0 warning branch a few lines below was unreachable dead code, and the padding step further down (which exists specifically to let users type values into a header-only upload) could never run. Verified with pd.read_csv(io.StringIO("num,grp,label\n"))shape=(0, 3), empty=True. Changed the guard to key only on len(df.columns) == 0, so genuinely columnless input is still rejected but header-only input loads and gets padded as before.

2. The new ValueError never reached the user (confirmed bug). All three callers (load_csv, load_excel, load_from_paste) wrapped _apply_loaded_df in a bare except Exception, which caught the new ValueError and replaced it with the generic "check the format" message. Added except ValueError as e: return False, str(e) above the catch-all in all three so the specific diagnostic surfaces.

Minor: added the missing test coverage and cleaned up the trailing whitespace, both as suggested.

New regression tests in tests/test_data_manager.py:

  • test_load_csv_with_header_only_still_loads_and_pads_rows — fails without the fix (used to reject with "Unable to load CSV file...")
  • test_apply_loaded_df_rejects_dataframe_with_no_columns
  • test_load_csv_with_no_columns_reports_specific_error_not_generic_one — fails without the fix (used to get the generic message)

Checks: pytest -q → 12 passed (was 8, +4 new... actually +3 net after accounting for the existing suite; all green). No mypy step exists in .github/workflows/ci.yml for this repo.

Nothing to push back on — both findings were accurate and reproducible.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions

Copy link
Copy Markdown
Contributor

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; pr-janitor.yml will close it if it goes stale.

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.

1 participant