|
Reference comment: #5130 (comment) |
Sorry, something went wrong.
|
One unstable unit test seems to be related to HF @franciscojavierarceo : And rerun got new error: |
Sorry, something went wrong.
|
Got it, I can make a patch for that. 👍 |
Sorry, something went wrong.
|
Got it, I can make a patch for that. 👍 No rush, take your time. This pr is ready for review now :) |
Sorry, something went wrong.
| ) -> dict[str, Union[list[Any], Any]]: | ||
| rand_dict_value: dict[ValueType, Union[list[Any], Any]] = { | ||
| ValueType.BYTES: [str.encode("hello world")], | ||
| ValueType.PDF_BYTES: [ |
There was a problem hiding this comment.
This is what's making the unit tests fail.
This is used in infer_features to validate the function schema / types actually.
Sorry, something went wrong.
There was a problem hiding this comment.
Ah I think I accidentally delete it when resolve merge conflict. let me add it back
Sorry, something went wrong.
| name=name if name is not None else user_function.__name__, | ||
| sources=sources, | ||
| schema=schema, | ||
| feature_transformation=transformation, |
There was a problem hiding this comment.
shouldn't we keep this to be backwards compatible?
Sorry, something went wrong.
There was a problem hiding this comment.
It is backwards compatible. I put the feature_transformation extraction logic inside the OnDemandFeatureView initialization, since the decorator doesn't pass in the feature_transformation param.
So user can do:
or
Sorry, something went wrong.
| from feast.transformation.mode import TransformationMode | ||
|
|
||
|
|
||
| class Transformation(ABC): |
There was a problem hiding this comment.
nit: I'd add a docstring here. ChatGPT should be a nice friend.
Sorry, something went wrong.
There was a problem hiding this comment.
That's good point
Sorry, something went wrong.
| owner: Optional[str] = "", | ||
| ): | ||
| def mainify(obj): | ||
| # Needed to allow dill to properly serialize the udf. Otherwise, clients will need to have a file with the same |
There was a problem hiding this comment.
Hmm, we encountered some serialization issues with dill in the past. @rostifar do you remember what they were?
Sorry, something went wrong.
There was a problem hiding this comment.
have dill issue as well. Created a new issue for it:
#5182
Sorry, something went wrong.
| ) | ||
| inferred_type = type(feature_value[0]) | ||
| inferred_value = feature_value[0] | ||
| if singleton and isinstance(inferred_value, list): |
There was a problem hiding this comment.
I just added this FYI as it's an edge case I didn't consider before, so please feel free to add it back in!
Sorry, something went wrong.
There was a problem hiding this comment.
strange I didn't modify this. not sure why it sneak in
Sorry, something went wrong.
There was a problem hiding this comment.
this look great! some small nits and some notes about fixing stuff I recently added which will fix the unit tests. otherwise looks awesome. 👏🚀🤠
Sorry, something went wrong.
|
this look great! some small nits and some notes about fixing stuff I recently added which will fix the unit tests. otherwise looks awesome. 👏🚀🤠 good catch. thanks! |
Sorry, something went wrong.
|
@franciscojavierarceo finally passed all tests! |
Sorry, something went wrong.
|
@franciscojavierarceo finally passed all tests! |
Sorry, something went wrong.
There was a problem hiding this comment.
Awesome work @HaoXuAI 🚀🚀🚀
Probably we can update the docs in a follow up PR before the next release?
Sorry, something went wrong.
What this PR does / why we need it:
Created a Transformation interface. it still works with the current pandas_transformation, python_transformation etc.
The next step is refactor the BatchMaterializationEngine to make it works for both Materialization and Transformation.
Which issue(s) this PR fixes:
#4584
#4277 (comment)
#4696
Misc