GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fix: Report Dask monitoring read errors instead of no data by ericdelorefice · Pull Request #6920 · feast-dev/feast

Conversation

@ericdelorefice

@ericdelorefice ericdelorefice commented Oct 1, 2026

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@ericdelorefice
ericdelorefice requested a review from a team as a code owner October 1, 2026 12:29
@ntkathole ntkathole added the ok-to-test label Oct 1, 2026
@codecov-commenter

codecov-commenter commented Oct 1, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ntkathole
ntkathole approved these changes Oct 1, 2026
View reviewed changes
@ericdelorefice @ntkathole
fix: Report Dask monitoring read errors instead of no data
134f2b5
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 <[email protected]>
@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

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ntkathole ntkathole ntkathole approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@ericdelorefice @codecov-commenter @ntkathole
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.