feat!: Refactor online stores by removing the contrib directory. (Breaking imports) #4731
Conversation
Signed-off-by: HaoXuAI <sduxuhao@gmail.com>
Signed-off-by: HaoXuAI <sduxuhao@gmail.com>
Signed-off-by: HaoXuAI <sduxuhao@gmail.com>
HaoXuAI
changed the title
Signed-off-by: HaoXuAI <sduxuhao@gmail.com>
franciscojavierarceo
left a comment
franciscojavierarceo
left a comment
There was a problem hiding this comment.
Looks like integration tests are failing?
seems passed now |
|
This will break any imports users are doing with custom code, can you update or title? |
Signed-off-by: HaoXuAI <sduxuhao@gmail.com>
Signed-off-by: HaoXuAI <sduxuhao@gmail.com>
There should be non-breaking changes, I also added the old custom type as well. |
I think he's saying that any feature store definition importing the legacy modules would be broken, e.g.: >> import feast.infra.online_stores.contrib.hazelcast_online_store.hazelcast_online_store
...
ModuleNotFoundError: No module named 'feast.infra.online_stores.contrib.hazelcast_online_store.hazelcast_online_store'Why don't you create aliases instead, with a warning to inform the user that these will be removed later? # feast/infra/online_stores/contrib/qdrant.py
import warnings
from feast.infra.online_stores.cqdrant import QdrantOnlineStore
warnings.warn(
"The module feast.infra.online_stores.contrib.qdrant is deprecated and will be removed in a future release. "
"Please use feast.infra.online_stores.cqdrant instead.",
DeprecationWarning,
stacklevel=2,
) |
| "qdrant": "feast.infra.online_stores.contrib.qdrant.QdrantOnlineStore", | ||
| "singlestore": "feast.infra.online_stores.singlestore_online_store.singlestore.SingleStoreOnlineStore", | ||
| "qdrant": "feast.infra.online_stores.cqdrant.QdrantOnlineStore", | ||
| **LEGACY_ONLINE_STORE_CLASS_FOR_TYPE, |
There was a problem hiding this comment.
IIUC, LEGACY_ONLINE_STORE_CLASS_FOR_TYPE would never be used unless someone declares a store like:
online_store:
type: feast.infra.online_stores.contrib.qdrant.QdrantOnlineStore
...Is this what you had in mind?
There was a problem hiding this comment.
agreed, I doubt anyone actually did anything like that. This is technically breaking, but I'm fine with it even w/o leaving aliases.
There was a problem hiding this comment.
Right that's what I'm referring to. I remember there was somewhere in doc said you can config online_store with something like: type: feast.infra.online_stores.contrib.qdrant.QdrantOnlineStore, but not sure if that's still the case.
Creating a alias means I have to keep two folders for same store. I think that's more confusing. |
Signed-off-by: HaoXuAI <sduxuhao@gmail.com>
|
@franciscojavierarceo @dmartinol fixed the conflict after new online store, need another review. |
franciscojavierarceo
changed the title
franciscojavierarceo
changed the title
…aking imports) (feast-dev#4731) * feat: Refactor online store by removing the contrib directory Signed-off-by: HaoXuAI <sduxuhao@gmail.com> * feat: Refactor online store by removing the contrib directory Signed-off-by: HaoXuAI <sduxuhao@gmail.com> * feat: Refactor online store by removing the contrib directory Signed-off-by: HaoXuAI <sduxuhao@gmail.com> * feat: Refactor online store by removing the contrib directory Signed-off-by: HaoXuAI <sduxuhao@gmail.com> * fix legacy mapping Signed-off-by: HaoXuAI <sduxuhao@gmail.com> * fix legacy mapping Signed-off-by: HaoXuAI <sduxuhao@gmail.com> --------- Signed-off-by: HaoXuAI <sduxuhao@gmail.com>
What this PR does / why we need it:
Which issue(s) this PR fixes:
Misc