Skip to content

fix: standardize error handling for bulk prep/correction reapply - #260

Open
iabaako wants to merge 7 commits into
mainfrom
fix/253-standardize-reapply-error-handling
Open

fix: standardize error handling for bulk prep/correction reapply#260
iabaako wants to merge 7 commits into
mainfrom
fix/253-standardize-reapply-error-handling

Conversation

@iabaako

@iabaako iabaako commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Pull Request Summary 🚀

What does this PR do? 📝

  • Standardizes error handling for bulk prep/correction reapply: a failing step is skipped and reported instead of crashing the page (prep) or silently vanishing (corrections). Closes Standardize error handling for failed prep/correction steps during bulk reapply #253.
  • Adds a status column (Successful/Failed, color-coded text) to both the Prep change log and the Correction log, refreshed every time a reapply runs.
  • Prep column-removal steps now tolerate partially-missing columns — remove what still exists and report the rest — instead of failing the whole step; the log message reflects the actual outcome (e.g. "2 columns removed" instead of the originally-requested 4).
  • Correction reapply now detects and reports three specific failure reasons instead of silently skipping or blindly applying a stale correction:
    1. the target column no longer exists in the data
    2. the current value has changed since the correction was logged (so blindly reapplying could clobber a legitimate update)
    3. the KEY is missing from the data
  • Fixes a caching bug where a newly added correction could take up to 30s (the cache TTL) to show up in the correction log.

Why is this change needed? 🤔

Prep and correction bulk-reapply previously handled per-item failures inconsistently and didn't match the UX already used for interactive single-item actions (see #253): a prep step that referenced a column dropped by a later re-import crashed the whole page, and a correction that failed to reapply was dropped with except Exception: pass and no user-facing signal. On top of that, there was no way to tell — after the fact — which steps/corrections in a log had actually applied successfully versus silently failed, which matters for data quality auditing.

How was this implemented? 🛠️

  • Added a shared datasure.utils.reapply_utils module (ReapplyFailure, warn_reapply_failures, highlight_status) so prep and corrections report and display failures the same way.
  • processing/prep.py: PrepProcessor.execute_all_actions now collects a PrepReapplyOutcome per action (status + error) instead of raising through; RemoveColumnsOperation splits requested columns into existing/missing instead of failing on any miss; the persisted prep log is rebuilt after each reapply with a fresh status/description per step while keeping each step's original request intact (so a column that reappears later is still requested for removal, and replication scripts stay accurate).
  • processing/corrections.py: _apply_correction_row now checks column existence and compares the current value against what was recorded at log time before applying (_current_value_mismatch); _reapply_all_corrections records status/status_reason per row and rewrites the correction log; add_correction_entry now clears the correction log/summary caches so new entries appear immediately, and marks new entries "Successful" (with backfill for logs saved before the status columns existed).
  • views/prep_view.py and views/correction_view.py: Change Log tables now select/display the status column right after action, styled via a shared pandas Styler helper (text color, not background — st.dataframe is kept over st.table so sorting/CSV download still work). correction_view.py's log-building logic was extracted into a plain _build_correction_log_display helper for direct unit testing, since render_correction_log is @st.fragment-wrapped and can't be exercised through the view-test mock.

How to test or reproduce? 🧪

  1. uv run python -m pytest — full suite passes.
  2. just lint-py / just fmt-python / uv tool run pre-commit run --all-files — clean.
  3. Manual: add a prep step removing multiple columns, then re-import raw data missing some of them — the step still removes the ones that exist and the log shows a Successful status with an updated description.
  4. Manual: add a correction (e.g. "remove row" for a given KEY), then re-import raw data without that KEY — the correction log's status flips to Failed with a reason, and a warning banner summarizes it. Verified end-to-end against real DuckDB-backed project data, not just mocks.

Screenshots (if applicable) 📷

Warning on import page after loading new dataset with missing columns or keys
Screenshot 2026-08-07 083712

New prep log with status
Screenshot 2026-08-07 083834

New corrections log with status and status reason
Screenshot 2026-08-07 083922

Checklist ✅

  • I have run and tested my changes locally
  • I have limit this PR to less than 1000 lines of code change (if not, explain why) — this PR is larger than 1000 lines because it bundles the original error-handling fix (Standardize error handling for failed prep/correction steps during bulk reapply #253) with the status-tracking follow-up work built in the same session; happy to split into separate PRs if preferred for review.
  • I have updated/added tests to cover my changes (if applicable)
  • I have updated/added requirements to cover my changes (if applicable) — N/A, no new dependencies
  • I have run linting and formatting on any code changes (if applicable)
  • I have updated the documentation (README, etc.) accordingly — N/A, no user-facing docs changes needed
  • I have reviewed and resolved any merge conflict

🤖 Generated with Claude Code

iabaako and others added 5 commits August 5, 2026 14:26
Prep reapply-all previously let a failing step raise uncaught, crashing
the page (e.g. a re-import that drops a column an earlier step used).
Correction reapply-all silently swallowed failures with a bare except.
Both now collect per-item failures, skip just the failing item, keep
applying the rest, and surface one shared st.warning at the UI boundary,
matching the existing pattern for interactive single-item actions.

Closes #253

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 7, 2026 08:42
@iabaako iabaako linked an issue Aug 7, 2026 that may be closed by this pull request
4 tasks
Comment thread tests/views/test_prep_view.py Fixed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR standardizes bulk reapply behavior for Prep steps and Corrections so that per-item failures are skipped and surfaced (rather than crashing Prep or silently disappearing in Corrections). It also adds a consistent status-based UX to both logs and fixes a correction-log caching lag after adding new corrections.

Changes:

  • Introduces a shared ReapplyFailure type plus shared UI helpers to warn once with a consolidated list of skipped items and to style status cells consistently.
  • Updates Prep and Corrections bulk reapply to continue on failures, persist refreshed log rows with Successful/Failed statuses, and return failure details to the UI layer.
  • Adds/updates tests to cover partial-failure scenarios, status backfilling for legacy logs, and the correction-log cache invalidation behavior.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/views/test_prep_view.py Adds a view test asserting the Prep Change Log renders the new status column and preserves column ordering.
tests/views/test_correction_view.py Adds unit tests for _build_correction_log_display (status backfill + column ordering).
tests/utils/test_reapply_utils.py Adds tests for shared status styling and the consolidated warning banner helper.
tests/utils/test_prep_utils.py Adds coverage for updated remove-columns messaging when some requested columns are missing.
tests/processing/test_prep.py Expands prep processing tests for partial missing column removal and bulk reapply outcomes (success/failure/continuation).
tests/processing/test_corrections.py Expands corrections processing tests for status logging, cache invalidation, and multiple failure reasons during bulk reapply.
src/datasure/views/prep_view.py Displays Prep Change Log with a status column and styled status cells; surfaces bulk reapply failures after step removal.
src/datasure/views/import_view.py Collects and warns about downstream prep/correction reapply failures after import refreshes.
src/datasure/views/correction_view.py Adds a display helper to backfill/order status columns and styles the Correction Log status cells; surfaces reapply failures after removal.
src/datasure/utils/reapply_utils.py New shared helpers: ReapplyFailure, highlight_status, and warn_reapply_failures.
src/datasure/utils/prep_utils.py Updates remove-columns confirmation message generation to reflect partial removals and missing-column skips.
src/datasure/processing/prep.py Implements per-action outcomes for bulk reapply, partial missing-column tolerance for removals, and persists refreshed prep logs with status.
src/datasure/processing/corrections.py Implements correction reapply failure detection/reporting, persists refreshed correction logs with status + reason, and clears cached log/summary on updates.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…rt' and 'import from''

Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 7, 2026 09:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/datasure/views/import_view.py:121

  • In the bulk-import flow, the status label is always set to "Data loaded successfully!" even when downstream reapply failures were collected and a warning is shown right after. This is user-facing and can be misleading during partial failure scenarios; consider updating the label when all_failures is non-empty.
        status.update(
            label="Data loaded successfully!", state="complete", expanded=True
        )

src/datasure/processing/corrections.py:704

  • For unexpected exceptions while applying a correction row, returning str(e) can yield an empty string and loses the exception type. Including the exception class name makes the failure reason clearer in status_reason and the aggregated warning.
        except Exception as e:
            # Skip corrections that fail (data may have changed)
            return data, str(e)

src/datasure/processing/prep.py:896

  • When a prep action fails with an unexpected exception, the stored error is str(e), which can be empty and omits the exception type. Including the exception class name makes the refreshed log and warning banner much more actionable for debugging.
            except Exception as e:
                outcomes.append(
                    PrepReapplyOutcome(
                        prep_args=action.prep_args, status="Failed", error=str(e)
                    )

Copilot AI review requested due to automatic review settings August 7, 2026 09:25
"""Change Log renders a status column and styles a Failed row."""
import importlib

import datasure.views.prep_view as pv_mod

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/datasure/processing/prep.py:1102

  • When building ReapplyFailure entries for failed prep steps, step=_generate_action_description(outcome.prep_args) will often produce a successful “✓ … removed … remaining” message (and may include remaining_count=None) because failed outcomes keep the original logged prep_args without counts. This makes the warning banner confusing/misleading for failures. Consider using a request-oriented description (e.g., action + requested columns) and ensure reason is never None.
    return [
        ReapplyFailure(
            step=_generate_action_description(outcome.prep_args), reason=outcome.error
        )
        for outcome in outcomes
        if outcome.status == "Failed"
    ]

@iabaako
iabaako marked this pull request as ready for review August 7, 2026 09:28
@iabaako
iabaako requested a review from a team as a code owner August 7, 2026 09:28
@iabaako
iabaako force-pushed the fix/253-standardize-reapply-error-handling branch from b844d3d to 11b69e2 Compare August 7, 2026 15:42
@sonarqubecloud

sonarqubecloud Bot commented Aug 7, 2026

Copy link
Copy Markdown

@wkirui wkirui left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This works as expected!

One quirk that I noticed and which is already in the pipeline for fixing is inverted data transformation steps: Removing row by condition - equal to and not equal to functionalities are inverted.

Image

A non-related issue found: When I upload two data sources, I found the following error.

2026-08-12 16:04:09.076 Uncaught app execution
Traceback (most recent call last):
  File "C:\Users\Wesley\Desktop\Projects\Python\datasure\.venv\Lib\site-packages\streamlit\runtime\scriptrunner\exec_code.py", line 129, in exec_func_with_error_handling
    result = func()
             ^^^^^^
  File "C:\Users\Wesley\Desktop\Projects\Python\datasure\.venv\Lib\site-packages\streamlit\runtime\scriptrunner\script_runner.py", line 789, in code_to_exec
    exec(code, module.__dict__)  # noqa: S102
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Wesley\Desktop\Projects\Python\datasure\src\datasure\app.py", line 210, in <module>
    nav_menu.run()
  File "C:\Users\Wesley\Desktop\Projects\Python\datasure\.venv\Lib\site-packages\streamlit\navigation\page.py", line 490, in run
    exec(code, module.__dict__)  # noqa: S102
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Wesley\Desktop\Projects\Python\datasure\src\datasure\views\import_view.py", line 342, in <module>
    render_local_file_form(project_id)
  File "C:\Users\Wesley\Desktop\Projects\Python\datasure\src\datasure\connectors\local.py", line 234, in render_local_file_form
    _handle_form_submission(
  File "C:\Users\Wesley\Desktop\Projects\Python\datasure\src\datasure\connectors\local.py", line 294, in _handle_form_submission
    import_log = pl.concat([import_log, new_row_df], how="vertical")
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Wesley\Desktop\Projects\Python\datasure\.venv\Lib\site-packages\polars\functions\eager.py", line 287, in concat
    out = wrap_df(plr.concat_df(elems))
                  ^^^^^^^^^^^^^^^^^^^^
polars.exceptions.ShapeError: unable to vstack, column names don't match: "source" and "alias"

How I got here:

  • Load a server dataset from SurveyCTO
  • Load a separate local csv file

Actually trying to reproduce this, the data prep page shows that the data from the local file don't exist yet I have loaded it.
Claude suggested changing this line in src/connectors/local.y:
import_log = pl.concat([import_log, new_row_df], how="vertical") to
import_log = pl.concat([import_log, new_row_df], how="diagonal") or how = diagonal_relaxed

Image

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.

Standardize error handling for failed prep/correction steps during bulk reapply

3 participants