[APPEND][FSTORE-1424] Feature logging for spark #1367

kennethmhc · 2024-07-09T06:43:37Z

This PR adds/fixes/changes...

please summarize your changes to the code
and make sure to include all changes to user-facing APIs

JIRA Issue: -

Priority for Review: -

Related PRs: -

How Has This Been Tested?

Unit Tests
Integration Tests
Manual Tests on VM

Checklist For The Assigned Reviewer:

- [ ] Checked if merge conflicts with master exist
- [ ] Checked if stylechecks for Java and Python pass
- [ ] Checked if all docstrings were added and/or updated appropriately
- [ ] Ran spellcheck on docstring
- [ ] Checked if guides & concepts need to be updated
- [ ] Checked if naming conventions for parameters and variables were followed
- [ ] Checked if private methods are properly declared and used
- [ ] Checked if hard-to-understand areas of code are commented
- [ ] Checked if tests are effective
- [ ] Built and deployed changes on dev VM and tested manually
- [x] (Checked if all type annotations were added and/or updated appropriately)

javierdlrm · 2024-07-15T08:23:50Z

python/hsfs/core/feature_view_api.py

@@ -460,4 +460,4 @@ def delete_feature_logs(
                path_params += [self._TRANSFORMED_lOG]
            else:
                path_params += [self._UNTRANSFORMED_LOG]
-        _client._send_request("DELETE", path_params, {})
+        return _client._send_request("DELETE", path_params, {})


What is this returning? Normally delete methods return void since the corresponding item gets deleted.

javierdlrm · 2024-07-15T08:26:21Z

python/hsfs/core/feature_logging.py

+    def update(self, others):
+        self._transformed_features = others._transformed_features
+        self._untransformed_features = others._untransformed_features
+        return self
+


Why this method instead of the setter methods for the two fields? @transformed_features.setter and @untransformed_features.setter?

javierdlrm · 2024-07-15T08:37:09Z

python/hsfs/core/feature_view_engine.py

    def _get_logging_fg(self, fv, transformed):
        feature_logging = self.get_feature_logging(fv)
+        return self._get_logging_fg_feature_logging(feature_logging, transformed)
+
+    def _get_logging_fg_feature_logging(self, feature_logging, transformed):
        if transformed:
            return feature_logging.transformed_features
        else:
            return feature_logging.untransformed_features


I think these list are not easy to read.
_get_logging_fg_feature_logging is returning features that can be transformed or untransformed, but the method name indicates feature_logging.
_get_logging_fg retrieves the feature_logging for a fv and then call the previous method to return the (transformed or untransformed) features.

I wouldn't guess their behavior based on the method names :)

Actually, based on their used in log_features, it seems that untransformed_features and transformed_features are actually fg objects

javierdlrm · 2024-07-15T08:41:59Z

python/hsfs/core/feature_view_engine.py

+            td_col_name=FeatureViewEngine._LOG_TD_VERSION,
+            time_col_name=FeatureViewEngine._LOG_TIME,
+            model_col_name=FeatureViewEngine._HSML_MODEL,
+            prediction=prediction,


Is prediction one or multiple predictions? If many, can we use predictions instead?

javierdlrm · 2024-07-15T08:49:14Z

python/hsfs/core/feature_view_engine.py

        default_write_options = {
            "start_offline_materialization": False,
        }
        if write_options:
            default_write_options.update(write_options)
-        fg = self._get_logging_fg(fv, transformed)
+        fg = self._get_logging_fg_feature_logging(feature_logging, transformed)


Could we better have a method called get_feature_group in the feature_logging class, that accepts a transformed boolean parameter?

def get_feature_group(self, transformed): return self._transformed_features if transformed else self._untransformed_features

Then:

we don't need _get_logging_fg_feature_logging and _get_logging_fg methods. We can just call get_feature_logging from the engine and then the internal method if needed.

we don't need to pass feature_logging as a separate parameter. We can use get_feature_logging(fv).get_feature_group(transformed)

javierdlrm · 2024-07-15T09:01:30Z

python/hsfs/engine/python.py

-                               td_col_name,
-                               time_col_name,
-                               model_col_name,
+    def get_feature_logging_df(features,


Can we add types to all parameters?

javierdlrm · 2024-07-15T09:07:40Z

python/hsfs/engine/spark.py

+    def get_feature_logging_df(self, features,
+                               fg=None,
+                               fg_features: List[TrainingDatasetFeature]=None,
+                               td_predictions: List[TrainingDatasetFeature]=None,
+                               td_col_name=None,
+                               time_col_name=None,
+                               model_col_name=None,
+                               prediction=None,
+                               training_dataset_version=None,
+                               hsml_model=None,
+                               **kwargs):


Can we add types to these parameters?

javierdlrm · 2024-07-15T09:08:47Z

python/hsfs/engine/spark.py

+        df = df.withColumn(td_col_name,
+                                       lit(training_dataset_version).cast(LongType()))


Indentation seems off here.

javierdlrm · 2024-07-15T09:09:44Z

python/hsfs/engine/spark.py

+        df = df.withColumn(time_col_name,
+                                       lit(now).cast(TimestampType()))


Indentation here as well

javierdlrm · 2024-07-15T09:15:54Z

python/hsfs/feature_view.py

@@ -3640,7 +3643,11 @@ def delete_log(self, transformed: Optional[bool]=None):
         # Raises
             `hsfs.client.exceptions.RestAPIError` in case the backend fails to delete the log.
         """
-        return self._feature_view_engine.delete_feature_logs(self, transformed)
+        if self._feature_logging is None:
+            self._feature_logging = self._feature_view_engine.get_feature_logging(self)


What if delete_log is called in a new FV without feature_logging enabled? (no call to enable_feature_logging() or log()) ?
I guess this line will return None, and then fail in the delete_feature_logs() ?

kennethmhc · 2024-07-15T09:54:31Z

@javierdlrm will close this PR and made a new one to the hopswork-api repo

kennethmhc added 8 commits July 9, 2024 08:24

handle null features in fg insert

8b3a6e4

update logging, get_feature_logging_df in spark

d0d826d

spark read log

817077a

fix delete log

f3b8093

log spark

7994d17

fix style

5099d5d

fix _apply_transformation_function

16e40f7

fix inplace

fb6642d

manu-sj approved these changes Jul 9, 2024

View reviewed changes

javierdlrm requested changes Jul 15, 2024

View reviewed changes

kennethmhc closed this Jul 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APPEND][FSTORE-1424] Feature logging for spark #1367

[APPEND][FSTORE-1424] Feature logging for spark #1367

kennethmhc commented Jul 9, 2024

javierdlrm Jul 15, 2024

javierdlrm Jul 15, 2024

javierdlrm Jul 15, 2024

javierdlrm Jul 15, 2024

javierdlrm Jul 15, 2024

javierdlrm Jul 15, 2024

javierdlrm Jul 15, 2024

javierdlrm Jul 15, 2024

javierdlrm Jul 15, 2024

javierdlrm Jul 15, 2024

javierdlrm Jul 15, 2024

kennethmhc commented Jul 15, 2024

		df = df.withColumn(td_col_name,
		lit(training_dataset_version).cast(LongType()))

		df = df.withColumn(time_col_name,
		lit(now).cast(TimestampType()))

[APPEND][FSTORE-1424] Feature logging for spark #1367

[APPEND][FSTORE-1424] Feature logging for spark #1367

Conversation

kennethmhc commented Jul 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennethmhc commented Jul 15, 2024