← 输入框|github.com|刷新
Skip to content

Allow for registering feature that doesn't have warehouse store #5

Closed

Description

@pradithya

Registering feature that doesn't have warehouse store will currently return error.
There is a use case that some feature doesn't need warehouse store, so the restriction could be removed.
Additionally, ingestion will need to be updated to do no operation for warehouse storing operation.

Activity

  1. woop commented on Dec 14, 2018

    @woop
    Member

    So the question here is

    • Do we allow toggling of the warehouse storage option at the feature level
    • or at the system level (Feast deployment)
    • or both?

    I think we should have the option to both disable warehouse storage on specific features, as well as deploy a Feast instance without a warehouse (serving only)

  2. tims commented on Dec 26, 2018

    @tims
    Contributor

    My pull request fixes a typo.. But basically ingestion already supports setting a "no operation" store, for either serving or ingestion. The core api would still need to implement sending it of course.

  3. woop commented on Jan 3, 2019

    @woop
    Member

    My pull request fixes a typo.. But basically ingestion already supports setting a "no operation" store, for either serving or ingestion. The core api would still need to implement sending it of course.

    So the next step is a PR which adds this functionality to core?

    @zhilingc, possible to have a look at this?

  4. tims commented on Jan 3, 2019

    @tims
    Contributor

    I'm also working on a PR to make ingestion accept an empty storage spec. Though in reality, that will interpreted as a StorageSpec.defaultInstance(). With empty string as the storeid. Ingestion will then treat this as a noop store.

  5. self-assigned this
    on Jan 3, 2019
  6. zhilingc commented on Jan 3, 2019

    @zhilingc
    Collaborator

    Ok. You might want to remove the validation check that makes declaring a store mandatory when registering a feature as well.

  7. tims commented on Jan 3, 2019

    @tims
    Contributor

    Aren't you worried people will register features and they will just all go to noop when they leave it out though? We do need to implement inheriting a system default first.

    And then a user needs to be able to override the default to a noop.

    Which now that I think about might be a pain. As if they set the store id to an empty string, it will look the same as the default instance, so how will you know when to override it with the default or not?

    Remember protos not having optional values means that there's no such thing as a null store id.

  8. woop commented on Jan 3, 2019

    @woop
    Member

    Aren't you worried people will register features and they will just all go to noop when they leave it out though? We do need to implement inheriting a system default first.

    And then a user needs to be able to override the default to a noop.

    Which now that I think about might be a pain. As if they set the store id to an empty string, it will look the same as the default instance, so how will you know when to override it with the default or not?

    Remember protos not having optional values means that there's no such thing as a null store id.

    Isn't a better approach simply to have an option to switch off either store? That we we don't have to look at the value and try to guess whether it should be on or off. I think the default should be inheritance.

  9. zhilingc commented on Jan 3, 2019

    @zhilingc
    Collaborator

    When is the default declared? Should it be in the configuration when deploying core Feast? Or is it a necessary step that has to be done by the user before registering features (but after deployment)?

  10. woop commented on Jan 3, 2019

    @woop
    Member

    When is the default declared? Should it be in the configuration when deploying core Feast? Or is it a necessary step that has to be done by the user before registering features (but after deployment)?

    I think it should be done during initial installation. That way the user could theoretically have all data storage abstracted away from them.

  11. tims commented on Jan 3, 2019

    @tims
    Contributor

    @zhilingc yes it should be a system default. So it should come with the system.

    Users shouldn't even be creating new stores? I think we made store specs creatable accidentally to be honest, because all the others are. They should probably only ever be created by the person who deploys a feast instance.

  12. tims commented on Jan 9, 2019

    @tims
    Contributor

    Can we close this issue now?

  13. tims commented on Jan 9, 2019

    @tims
    Contributor

    Closing, but reopen if I missed something.

  14. 5 remaining items

  15. pradithya commented on Jan 18, 2019

    @pradithya
    CollaboratorAuthor

    Apparently the ingestion job succeed but all feature row didn't pass validation

  16. added a commit that references this issue on Nov 25, 2021
  17. added a commit that references this issue on May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions