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.

Sign up for GitHub

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

Jump to bottom

fix: Removed version func from feature_store.py #4748

Merged
merged 1 commit into from
Nov 7, 2024
Merged

fix: Removed version func from feature_store.py #4748

merged 1 commit into from
Nov 7, 2024

Conversation

dandawg
Copy link
Contributor

@dandawg dandawg commented Nov 7, 2024

What this PR does / why we need it:

This PR removes the version function from feature_store.py, which is a method to the FeatureStore class. The version method was unused, un-needed, and confusing. See issue #4747 for more details.

I categorized this as a "fix" because I see an unused/un-needed function as a bug, but I understand that this could merit a different categorization.

Which issue(s) this PR fixes:

#4747

@dandawg
removed version func from feature_store.py
f2353c9 
Signed-off-by: dandawg <12484302+dandawg@users.noreply.github.com>
@dandawg dandawg requested a review from a team as a code owner November 7, 2024 21:16
shuchu
shuchu approved these changes Nov 7, 2024
View reviewed changes
Copy link
Collaborator

@shuchu shuchu left a comment

Choose a reason for hiding this comment

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

Lgtm

@dandawg
Copy link
Contributor Author

dandawg commented Nov 7, 2024

I used vscode's 'references' functionality to verify that this function was unused in the repo. To double check this, I also did the following:

# search for all mentions of version(
# excluding ones like ..._version(, ...conversion(, etc.
# also excludes lines with venv (my virtual env files)
grep -R '[^A-z_]version(' . | grep -v "venv"

Output:

./sdk/python/feast/feature_store.py:    def version(self) -> str:
./sdk/python/feast/version.py:        sdk_version = version("feast")
./sdk/python/feast/cli.py:def version():
./build/lib/feast/feature_store.py:    def version(self) -> str:
./build/lib/feast/version.py:        sdk_version = version("feast")
./build/lib/feast/cli.py:def version():

I manually verified that ./sdk/python/feast/version.py and ./build/lib/feast/version.py are not using feature_store.FeatureStore.version.

franciscojavierarceo
franciscojavierarceo approved these changes Nov 7, 2024
View reviewed changes
Copy link
Member

@franciscojavierarceo franciscojavierarceo left a comment

Choose a reason for hiding this comment

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

Yeah I can see how this is convenient but I definitely agree this should just rely on package version.

Thank you!

@franciscojavierarceo franciscojavierarceo enabled auto-merge (squash) November 7, 2024 21:42
@franciscojavierarceo franciscojavierarceo merged commit f902bb9 into feast-dev:master Nov 7, 2024
25 checks passed
@dandawg dandawg deleted the remove-version-func branch November 8, 2024 20:15
shuchu pushed a commit to shuchu/feast that referenced this pull request Nov 21, 2024
@dandawg @shuchu
fix: Removed version func from feature_store.py (feast-dev#4748)
746bade
franciscojavierarceo pushed a commit that referenced this pull request Dec 5, 2024
@feast-ci-bot
chore(release): release 0.42.0
aac334a 
# [0.42.0](v0.41.0...v0.42.0) (2024-12-05)

### Bug Fixes

* Add adapters for sqlite datetime conversion ([#4797](#4797)) ([e198b17](e198b17))
* Added grpcio extras to default feature-server image ([#4737](#4737)) ([e9cd373](e9cd373))
* Changing node version in release ([7089918](7089918))
* Feast create empty online table when FeatureView attribute online=False ([#4666](#4666)) ([237c453](237c453))
* Fix db store types in Operator CRD ([#4798](#4798)) ([f09339e](f09339e))
* Fix the config issue for postgres ([#4776](#4776)) ([a36f7e5](a36f7e5))
* Fixed example materialize-incremental and improved explanation ([#4734](#4734)) ([ca8a7ab](ca8a7ab))
* Fixed SparkSource docstrings so it wouldn't used inhereted class docstrings ([#4722](#4722)) ([32e6aa1](32e6aa1))
* Fixing PGVector integration tests ([#4778](#4778)) ([88a0320](88a0320))
* Incorrect type passed to assert_permissions in materialize endpoints ([#4727](#4727)) ([b72c2da](b72c2da))
* Issue of DataSource subclasses using parent abstract class docstrings ([#4730](#4730)) ([b24acd5](b24acd5))
* Operator envVar positioning & tls.SecretRef.Name ([#4806](#4806)) ([1115d96](1115d96))
* Populates project created_time correctly according to created ti… ([#4686](#4686)) ([a61b93c](a61b93c))
* Reduce feast-server container image size & fix dev image build ([#4781](#4781)) ([ccc9aea](ccc9aea))
* Removed version func from feature_store.py ([#4748](#4748)) ([f902bb9](f902bb9))
* Support registry instantiation for read-only users ([#4719](#4719)) ([ca3d3c8](ca3d3c8))
* Syntax Error in BigQuery While Retrieving Columns that Start wit… ([#4713](#4713)) ([60fbc62](60fbc62))
* Update release version in a pertinent Operator file ([#4708](#4708)) ([764a8a6](764a8a6))

### Features

* Add api contract to fastapi docs ([#4721](#4721)) ([1a165c7](1a165c7))
* Add Couchbase as an online store ([#4637](#4637)) ([824859b](824859b))
* Add Operator support for spec.feastProject & status.applied fields ([#4656](#4656)) ([430ac53](430ac53))
* Add services functionality to Operator ([#4723](#4723)) ([d1d80c0](d1d80c0))
* Add TLS support to the Operator ([#4796](#4796)) ([a617a6c](a617a6c))
* Added feast Go operator db stores support ([#4771](#4771)) ([3302363](3302363))
* Added support for setting env vars in feast services in feast controller  ([#4739](#4739)) ([84b24b5](84b24b5))
* Adding docs outlining native Python transformations on singletons ([#4741](#4741)) ([0150278](0150278))
* Adding first feast operator e2e test. ([#4791](#4791)) ([8339f8d](8339f8d))
* Adding github action to run the operator end-to-end tests. ([#4762](#4762)) ([d8ccb00](d8ccb00))
* Adding ssl support for registry server. ([#4718](#4718)) ([ccf7a55](ccf7a55))
* Adding SSL support for the React UI server and feast UI command. ([#4736](#4736)) ([4a89252](4a89252))
* Adding support for native Python transformations on a single dictionary  ([#4724](#4724)) ([9bbc1c6](9bbc1c6))
* Adding TLS support for offline server. ([#4744](#4744)) ([5d8d03f](5d8d03f))
* Building the feast image ([#4775](#4775)) ([6635dde](6635dde))
* File persistence definition and implementation ([#4742](#4742)) ([3bad4a1](3bad4a1))
* Object store persistence in operator ([#4758](#4758)) ([0ae86da](0ae86da))
* OIDC authorization in Feast Operator ([#4801](#4801)) ([eb111d6](eb111d6))
* Operator will create k8s serviceaccount for each feast service ([#4767](#4767)) ([cde5760](cde5760))
* Printing more verbose logs when we start the offline server  ([#4660](#4660)) ([9d8d3d8](9d8d3d8))
* PVC configuration and impl ([#4750](#4750)) ([785a190](785a190))
* Qdrant vectorstore support ([#4689](#4689)) ([86573d2](86573d2))
* RBAC Authorization in Feast Operator ([#4786](#4786)) ([0ef5acc](0ef5acc))
* Support for nested timestamp fields in Spark Offline store ([#4740](#4740)) ([d4d94f8](d4d94f8))
* Update the go feature server from Expedia code repo. ([#4665](#4665)) ([6406625](6406625))
* Updated feast Go operator db stores ([#4809](#4809)) ([2c5a6b5](2c5a6b5))
* Updated sample secret following review ([#4811](#4811)) ([dc9f825](dc9f825))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@franciscojavierarceo franciscojavierarceo franciscojavierarceo approved these changes

@shuchu shuchu shuchu approved these changes

Assignees
No one assigned
Labels
ok-to-test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dandawg @shuchu @franciscojavierarceo