Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: OnDemandFeatureView type inference for array types #4310

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

alexmirrington
Copy link
Contributor

@alexmirrington alexmirrington commented Jun 24, 2024

What and Why

OnDemandFeatureView.feature_transformation.infer_features should be able to infer features from python types for all supported feast data types, for all transformation backends. This PR passes values to python_type_to_feast_value_type so that list types can be inferred correctly.

Tests

  • Unit tests passing in CI for all Python versions on my fork.
  • Tested against a local project by patching PandasTransformation.infer_features, e.g.
from feast.transformation.pandas_transformation import PandasTransformation

def __patched_infer_features(
    self, random_input: dict[str, list[Any]]
) -> list[Field]:
    df = pd.DataFrame.from_dict(random_input)
    output_df: pd.DataFrame = self.transform(df)
    return [
        Field(
            name=f,
            dtype=from_value_type(
                python_type_to_feast_value_type(
                    f, value=output_df[f].tolist()[0], type_name=str(dt)
                )
            ),
        )
        for f, dt in zip(output_df.columns, output_df.dtypes)
    ]

PandasTransformation.infer_features = __patched_infer_features

Note to reviewers
If you can point me to where I could add some feature repository tests to catch any regressions in the future that would be great 🙏

Fixes

Fixes #4308

@alexmirrington alexmirrington changed the title Fix OnDemandFeatureView type inference for array types fix: OnDemandFeatureView type inference for array types Jun 24, 2024
@alexmirrington alexmirrington force-pushed the fix-odfv-type-inference branch from feac39c to 6b7afb9 Compare June 24, 2024 10:45
@alexmirrington alexmirrington marked this pull request as ready for review June 24, 2024 10:49
@tokoko
Copy link
Collaborator

tokoko commented Jun 24, 2024

I think the best way for this to be tested would be in respective unit tests: test_on_demand_pandas_transformation.py, test_on_demand_python_transformation.py and test_substrait_transformation.py

@alexmirrington alexmirrington force-pushed the fix-odfv-type-inference branch 3 times, most recently from 9594997 to 62208ea Compare June 25, 2024 06:18
@alexmirrington
Copy link
Contributor Author

@tokoko Added some tests for lists of bools, strings, floats and ints for both python and pandas ODFVs, as well as a request source in there. LMK what you think!

@alexmirrington
Copy link
Contributor Author

alexmirrington commented Jun 27, 2024

@HaoXuAI FYI seeing a few rate limits for bigtable and redshift in integration tests, doesn't seem related to my changes but maybe you know who best could take a look?

@tokoko
Copy link
Collaborator

tokoko commented Jun 28, 2024

@alexmirrington Can you make a dummy commit to force a rerun?
@HaoXuAI Some tests still need First-time contributor approval

Copy link
Collaborator

@tokoko tokoko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alexmirrington alexmirrington force-pushed the fix-odfv-type-inference branch 2 times, most recently from 92a3441 to 44ed226 Compare July 2, 2024 13:16
@alexmirrington
Copy link
Contributor Author

alexmirrington commented Jul 2, 2024

Can you make a dummy commit to force a rerun?

@tokoko @HaoXuAI Just pushed some validation checks and tests as requested, should be good for final review now

@alexmirrington alexmirrington requested a review from HaoXuAI July 2, 2024 13:19
Signed-off-by: Alex Mirrington <alex.mirrington@rokt.com>
@alexmirrington alexmirrington force-pushed the fix-odfv-type-inference branch from 44ed226 to 0ff73fb Compare July 7, 2024 01:37
@alexmirrington
Copy link
Contributor Author

Ready to merge if you are happy, just need permissions to run some final checks now that I've fixed linting issues.
cc @HaoXuAI

@tokoko tokoko merged commit c45ff72 into feast-dev:master Jul 10, 2024
16 checks passed
tsisodia10 pushed a commit to tsisodia10/feast that referenced this pull request Jul 23, 2024
Fix OnDemandFeatureView type inference for array types

Signed-off-by: Alex Mirrington <alex.mirrington@rokt.com>
tsisodia10 pushed a commit to tsisodia10/feast that referenced this pull request Jul 23, 2024
Fix OnDemandFeatureView type inference for array types

Signed-off-by: Alex Mirrington <alex.mirrington@rokt.com>
nick-amaya-sp pushed a commit to sailpoint/feast that referenced this pull request Jul 23, 2024
Fix OnDemandFeatureView type inference for array types

Signed-off-by: Alex Mirrington <alex.mirrington@rokt.com>
franciscojavierarceo pushed a commit that referenced this pull request Jul 31, 2024
# [0.40.0](v0.39.0...v0.40.0) (2024-07-31)

### Bug Fixes

* Added missing type ([#4315](#4315)) ([86af60a](86af60a))
* Avoid XSS attack from Jinjin2's Environment(). ([#4355](#4355)) ([40270e7](40270e7))
* CGO Memory leak issue in GO Feature server ([#4291](#4291)) ([43e198f](43e198f))
* Deprecated the datetime.utcfromtimestamp(). ([#4306](#4306)) ([21deec8](21deec8))
* Fix SQLite import issue ([#4294](#4294)) ([398ea3b](398ea3b))
* Increment operator to v0.39.0 ([#4368](#4368)) ([3ddb4fb](3ddb4fb))
* Minor typo in the unit test. ([#4296](#4296)) ([6c75e84](6c75e84))
* OnDemandFeatureView type inference for array types ([#4310](#4310)) ([c45ff72](c45ff72))
* Remove redundant batching in PostgreSQLOnlineStore.online_write_batch and fix progress bar ([#4331](#4331)) ([0d89d15](0d89d15))
* Remove typo. ([#4351](#4351)) ([92d17de](92d17de))
* Retire the datetime.utcnow(). ([#4352](#4352)) ([a8bc696](a8bc696))
* Update dask version to support pandas 1.x ([#4326](#4326)) ([a639d61](a639d61))
* Update Feast object metadata in the registry ([#4257](#4257)) ([8028ae0](8028ae0))
* Using one single function call for utcnow(). ([#4307](#4307)) ([98ff63c](98ff63c))

### Features

* Add async feature retrieval for Postgres Online Store ([#4327](#4327)) ([cea52e9](cea52e9))
* Add Async refresh to Sql Registry ([#4251](#4251)) ([f569786](f569786))
* Add SingleStore as an OnlineStore ([#4285](#4285)) ([2c38946](2c38946))
* Add Tornike to maintainers.md ([#4339](#4339)) ([8e8c1f2](8e8c1f2))
* Bump psycopg2 to psycopg3 for all Postgres components ([#4303](#4303)) ([9451d9c](9451d9c))
* Entity key deserialization ([#4284](#4284)) ([83fad15](83fad15))
* Ignore paths feast apply ([#4276](#4276)) ([b4d54af](b4d54af))
* Move get_online_features to OnlineStore interface ([#4319](#4319)) ([7072fd0](7072fd0))
* Port mssql contrib offline store to ibis ([#4360](#4360)) ([7914cbd](7914cbd))

### Reverts

* Revert "fix: Avoid XSS attack from Jinjin2's Environment()." ([#4357](#4357)) ([cdeab48](cdeab48)), closes [#4355](#4355)
redhatHameed pushed a commit to RHEcosystemAppEng/feast that referenced this pull request Aug 5, 2024
# [0.40.0](feast-dev/feast@v0.39.0...v0.40.0) (2024-07-31)

### Bug Fixes

* Added missing type ([feast-dev#4315](feast-dev#4315)) ([86af60a](feast-dev@86af60a))
* Avoid XSS attack from Jinjin2's Environment(). ([feast-dev#4355](feast-dev#4355)) ([40270e7](feast-dev@40270e7))
* CGO Memory leak issue in GO Feature server ([feast-dev#4291](feast-dev#4291)) ([43e198f](feast-dev@43e198f))
* Deprecated the datetime.utcfromtimestamp(). ([feast-dev#4306](feast-dev#4306)) ([21deec8](feast-dev@21deec8))
* Fix SQLite import issue ([feast-dev#4294](feast-dev#4294)) ([398ea3b](feast-dev@398ea3b))
* Increment operator to v0.39.0 ([feast-dev#4368](feast-dev#4368)) ([3ddb4fb](feast-dev@3ddb4fb))
* Minor typo in the unit test. ([feast-dev#4296](feast-dev#4296)) ([6c75e84](feast-dev@6c75e84))
* OnDemandFeatureView type inference for array types ([feast-dev#4310](feast-dev#4310)) ([c45ff72](feast-dev@c45ff72))
* Remove redundant batching in PostgreSQLOnlineStore.online_write_batch and fix progress bar ([feast-dev#4331](feast-dev#4331)) ([0d89d15](feast-dev@0d89d15))
* Remove typo. ([feast-dev#4351](feast-dev#4351)) ([92d17de](feast-dev@92d17de))
* Retire the datetime.utcnow(). ([feast-dev#4352](feast-dev#4352)) ([a8bc696](feast-dev@a8bc696))
* Update dask version to support pandas 1.x ([feast-dev#4326](feast-dev#4326)) ([a639d61](feast-dev@a639d61))
* Update Feast object metadata in the registry ([feast-dev#4257](feast-dev#4257)) ([8028ae0](feast-dev@8028ae0))
* Using one single function call for utcnow(). ([feast-dev#4307](feast-dev#4307)) ([98ff63c](feast-dev@98ff63c))

### Features

* Add async feature retrieval for Postgres Online Store ([feast-dev#4327](feast-dev#4327)) ([cea52e9](feast-dev@cea52e9))
* Add Async refresh to Sql Registry ([feast-dev#4251](feast-dev#4251)) ([f569786](feast-dev@f569786))
* Add SingleStore as an OnlineStore ([feast-dev#4285](feast-dev#4285)) ([2c38946](feast-dev@2c38946))
* Add Tornike to maintainers.md ([feast-dev#4339](feast-dev#4339)) ([8e8c1f2](feast-dev@8e8c1f2))
* Bump psycopg2 to psycopg3 for all Postgres components ([feast-dev#4303](feast-dev#4303)) ([9451d9c](feast-dev@9451d9c))
* Entity key deserialization ([feast-dev#4284](feast-dev#4284)) ([83fad15](feast-dev@83fad15))
* Ignore paths feast apply ([feast-dev#4276](feast-dev#4276)) ([b4d54af](feast-dev@b4d54af))
* Move get_online_features to OnlineStore interface ([feast-dev#4319](feast-dev#4319)) ([7072fd0](feast-dev@7072fd0))
* Port mssql contrib offline store to ibis ([feast-dev#4360](feast-dev#4360)) ([7914cbd](feast-dev@7914cbd))

### Reverts

* Revert "fix: Avoid XSS attack from Jinjin2's Environment()." ([feast-dev#4357](feast-dev#4357)) ([cdeab48](feast-dev@cdeab48)), closes [feast-dev#4355](feast-dev#4355)
shuchu pushed a commit to shuchu/feast that referenced this pull request Aug 14, 2024
# [0.40.0](feast-dev/feast@v0.39.0...v0.40.0) (2024-07-31)

### Bug Fixes

* Added missing type ([feast-dev#4315](feast-dev#4315)) ([86af60a](feast-dev@86af60a))
* Avoid XSS attack from Jinjin2's Environment(). ([feast-dev#4355](feast-dev#4355)) ([40270e7](feast-dev@40270e7))
* CGO Memory leak issue in GO Feature server ([feast-dev#4291](feast-dev#4291)) ([43e198f](feast-dev@43e198f))
* Deprecated the datetime.utcfromtimestamp(). ([feast-dev#4306](feast-dev#4306)) ([21deec8](feast-dev@21deec8))
* Fix SQLite import issue ([feast-dev#4294](feast-dev#4294)) ([398ea3b](feast-dev@398ea3b))
* Increment operator to v0.39.0 ([feast-dev#4368](feast-dev#4368)) ([3ddb4fb](feast-dev@3ddb4fb))
* Minor typo in the unit test. ([feast-dev#4296](feast-dev#4296)) ([6c75e84](feast-dev@6c75e84))
* OnDemandFeatureView type inference for array types ([feast-dev#4310](feast-dev#4310)) ([c45ff72](feast-dev@c45ff72))
* Remove redundant batching in PostgreSQLOnlineStore.online_write_batch and fix progress bar ([feast-dev#4331](feast-dev#4331)) ([0d89d15](feast-dev@0d89d15))
* Remove typo. ([feast-dev#4351](feast-dev#4351)) ([92d17de](feast-dev@92d17de))
* Retire the datetime.utcnow(). ([feast-dev#4352](feast-dev#4352)) ([a8bc696](feast-dev@a8bc696))
* Update dask version to support pandas 1.x ([feast-dev#4326](feast-dev#4326)) ([a639d61](feast-dev@a639d61))
* Update Feast object metadata in the registry ([feast-dev#4257](feast-dev#4257)) ([8028ae0](feast-dev@8028ae0))
* Using one single function call for utcnow(). ([feast-dev#4307](feast-dev#4307)) ([98ff63c](feast-dev@98ff63c))

### Features

* Add async feature retrieval for Postgres Online Store ([feast-dev#4327](feast-dev#4327)) ([cea52e9](feast-dev@cea52e9))
* Add Async refresh to Sql Registry ([feast-dev#4251](feast-dev#4251)) ([f569786](feast-dev@f569786))
* Add SingleStore as an OnlineStore ([feast-dev#4285](feast-dev#4285)) ([2c38946](feast-dev@2c38946))
* Add Tornike to maintainers.md ([feast-dev#4339](feast-dev#4339)) ([8e8c1f2](feast-dev@8e8c1f2))
* Bump psycopg2 to psycopg3 for all Postgres components ([feast-dev#4303](feast-dev#4303)) ([9451d9c](feast-dev@9451d9c))
* Entity key deserialization ([feast-dev#4284](feast-dev#4284)) ([83fad15](feast-dev@83fad15))
* Ignore paths feast apply ([feast-dev#4276](feast-dev#4276)) ([b4d54af](feast-dev@b4d54af))
* Move get_online_features to OnlineStore interface ([feast-dev#4319](feast-dev#4319)) ([7072fd0](feast-dev@7072fd0))
* Port mssql contrib offline store to ibis ([feast-dev#4360](feast-dev#4360)) ([7914cbd](feast-dev@7914cbd))

### Reverts

* Revert "fix: Avoid XSS attack from Jinjin2's Environment()." ([feast-dev#4357](feast-dev#4357)) ([cdeab48](feast-dev@cdeab48)), closes [feast-dev#4355](feast-dev#4355)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants