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 12 commits into
Conversation
Sorry, something went wrong.
Signed-off-by: Vanshika Vanshika <vvanshik@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
|
⚠️ Please install the Codecov Report❌ Patch coverage is 53.25248% with 424 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## master #6702 +/- ##
==========================================
+ Coverage 47.09% 47.31% +0.22%
==========================================
Files 419 424 +5
Lines 51878 52784 +906
Branches 7525 7675 +150
==========================================
+ Hits 24430 24974 +544
- Misses 25700 26017 +317
- Partials 1748 1793 +45
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
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
There was a problem hiding this comment.
Thanks for the feature! A few concerns — mostly around thread-safety and error handling.
Sorry, something went wrong.
|
|
||
|
|
||
| @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.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
Signed-off-by: Vanshika Vanshika <vvanshik@redhat.com>
Signed-off-by: Vanshika Vanshika <vvanshik@redhat.com>
Signed-off-by: Vanshika Vanshika <vvanshik@redhat.com>
What this PR does / why we need it:
Adds MlflowDatasetSource so Feast can read tabular MLflow data for offline retrieval via get_historical_features() and create_saved_dataset().
Which issue(s) this PR fixes:
Checks
Testing Strategy
Misc