feat: Add MLflow as a first-class offline DataSource for Feast - #6702
feat: Add MLflow as a first-class offline DataSource for Feast#6702Vperiodt wants to merge 6 commits into
Conversation
Signed-off-by: Vanshika Vanshika <vvanshik@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6702 +/- ##
==========================================
- Coverage 46.80% 46.74% -0.07%
==========================================
Files 415 420 +5
Lines 50397 51155 +758
Branches 7215 7347 +132
==========================================
+ Hits 23589 23913 +324
- Misses 25155 25548 +393
- Partials 1653 1694 +41
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Signed-off-by: Vanshika Vanshika <vvanshik@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
ntkathole
left a comment
ntkathole
left a comment
There was a problem hiding this comment.
Thanks for the feature! A few concerns — mostly around thread-safety and error handling.
|
|
||
|
|
||
| @contextlib.contextmanager | ||
| def mlflow_token_scope(token: Optional[str]) -> Iterator[None]: |
There was a problem hiding this comment.
os.environ is process-global — concurrent requests in a multi-threaded Feast server will race over MLFLOW_TRACKING_TOKEN, potentially leaking tokens between users. Same issue applies to mlflow.set_tracking_uri() calls throughout the PR.
| f"Unsupported artifact format: {self.artifact_format}" | ||
| ) | ||
|
|
||
| def _fetch_genai_dataframe(self): |
There was a problem hiding this comment.
This method never calls resolve_mlflow_token() or wraps in mlflow_token_scope(), so schema introspection will fail with 401 in authenticated deployments. Compare with to_arrow() which correctly handles auth.
| try: | ||
| store.write_to_offline_store(feature_view_name, batch) | ||
| except Exception as e: | ||
| logger.warning( |
There was a problem hiding this comment.
Offline write errors are logged but NOT added to result.errors, so the watermark can still advance even when offline data is lost. Those records become unrecoverable on the next incremental sync.
| assert "score" in result_df.columns | ||
| assert "category" in result_df.columns | ||
| except Exception: | ||
| pytest.skip( |
There was a problem hiding this comment.
Catching all Exception types and calling pytest.skip means this test can never fail — any bug (AssertionError, TypeError, etc.) is silently skipped. This provides zero regression safety.
| if view is None: | ||
| return None | ||
|
|
||
| for attr in ("stream_source", "source", "data_source"): |
There was a problem hiding this comment.
When FeatureView(source=MlflowDatasetSource(...)) is used, the source is stored as batch_source. This lookup checks stream_source, source, data_source but not batch_source.
3 participants
What this PR does / why we need it:
Adds
MlflowDatasetSourceso Feast can read tabular MLflow data for offline retrieval viaget_historical_features()andcreate_saved_dataset().dataset_name/dataset_id) and artifact (run_id+artifact_path, Parquet/CSV)to_arrow()fallback for other storessync-dataset,preview-dataset,validate-source,list-sourcesmlflowis not installed; non-MLflow FeatureViews unaffectedWhich issue(s) this PR fixes:
Checks
git commit -s)Testing Strategy
Misc