Skip to content

fix: Report Dask monitoring read errors instead of no data - #6920

Merged
ntkathole merged 1 commit into
feast-dev:masterfrom
ericdelorefice:dask-monitoring-read-errors
Oct 1, 2026
Merged

ntkathole merged 1 commit into
feast-dev:masterfrom
ericdelorefice:dask-monitoring-read-errors

Conversation

@ericdelorefice

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

Problem

DaskOfflineStore.get_monitoring_max_timestamp wraps pq.read_table in except Exception: return None, and None means "this source has no data". So a file that exists but cannot be read, or a storage error while reading it, is reported the same way as an empty source.

The callers act on that:

  • MonitoringService logs "No data found for feature view '%s', skipping" and skips the feature view, so an unreadable source looks like an empty one.
  • The baseline and compute paths pass the result as max_event_timestamp to _save_computed_metrics and _compute_for_feature_view, so the metrics are saved with no max_event_timestamp when the read failed.

compute_monitoring_metrics in the same store reads the same file without a handler, so it raises for the same failure.

Reproduction

A real parquet file in a temp repo, on master:

file with two rows               -> 2026-09-30 12:00:00+00:00
path that was never written      -> None
file containing b"not a parquet" -> None
read_table raising OSError       -> None

Change

Only FileNotFoundError returns None, since a source nothing has been written to has no data to monitor. Any other error reaches the caller, where MonitoringService already catches it per feature view and logs "Failed to auto-compute metrics for feature view".

I checked that a missing local path raises FileNotFoundError from pq.read_table with pyarrow 25.0.1. pyarrow's remote filesystems report a missing path the same way, but I did not run this against S3 or GCS.

Verification

New tests/unit/infra/offline_stores/test_dask_monitoring_max_timestamp.py, 4 tests. On master the corrupt-file and storage-error tests fail (DID NOT RAISE), and the 2 controls pass, a readable file and a missing path. With the change all 4 pass.

tests/unit/monitoring and test_dask_non_entity.py give the same 10 failures with and without the change, all in tests that need optional backends I don't have installed (BigQuery and others). ruff check and ruff format --check are clean. mypy reports 3 errors in dask.py at lines 183, 184 and 363, the same 3 as on master.

Which issue(s) this PR fixes:

None filed. Same class of fix as #6918, which does this for Trino's query_monitoring_metrics.

Checks

  • I've made sure the tests are passing.
  • My commits are signed off (git commit -s)
  • My PR title follows conventional commits format

Testing Strategy

  • Unit tests
  • Integration tests
  • Manual tests
  • Testing is not required for this change

@ericdelorefice
ericdelorefice requested a review from a team as a code owner October 1, 2026 12:29
@codecov-commenter

codecov-commenter commented Oct 1, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.58%. Comparing base (d169238) to head (134f2b5).
⚠️ Report is 1 commits behind head on master.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6920      +/-   ##
==========================================
+ Coverage   48.56%   48.58%   +0.02%     
==========================================
  Files         427      427              
  Lines       53787    53787              
  Branches     7834     7834              
==========================================
+ Hits        26119    26132      +13     
+ Misses      25803    25788      -15     
- Partials     1865     1867       +2     
Flag Coverage Δ
go-feature-server 30.58% <ø> (ø)
python-unit 49.95% <100.00%> (+0.02%) ⬆️
Files with missing lines Coverage Δ
sdk/python/feast/infra/offline_stores/dask.py 54.66% <100.00%> (+2.28%) ⬆️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d169238...134f2b5. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

get_monitoring_max_timestamp returned None for any exception from
pq.read_table, so an unreadable source was reported as having no data
and metrics were saved without max_event_timestamp. Only a missing file
now means no data; other errors reach the caller.

Signed-off-by: Eric Del Orefice <114540347+ericdelorefice@users.noreply.github.com>
@ntkathole
ntkathole force-pushed the dask-monitoring-read-errors branch from 3979347 to 134f2b5 Compare October 1, 2026 14:06
@ntkathole
ntkathole merged commit 81673d9 into feast-dev:master Oct 1, 2026
19 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants