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

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

Closed
pradithya opened this issue Dec 12, 2018 · 15 comments
Closed

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

pradithya opened this issue Dec 12, 2018 · 15 comments
Assignees
Labels
kind/feature New feature or request

Comments

@pradithya
Copy link
Collaborator

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.

@woop
Copy link
Member

woop commented Dec 14, 2018

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)

@tims
Copy link
Contributor

tims commented Dec 26, 2018

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.

@woop
Copy link
Member

woop commented Jan 3, 2019

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?

@tims
Copy link
Contributor

tims commented Jan 3, 2019

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.

@tims tims self-assigned this Jan 3, 2019
@zhilingc
Copy link
Collaborator

zhilingc commented Jan 3, 2019

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

@tims
Copy link
Contributor

tims commented Jan 3, 2019

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.

@woop
Copy link
Member

woop commented Jan 3, 2019

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.

@zhilingc
Copy link
Collaborator

zhilingc commented Jan 3, 2019

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)?

@woop
Copy link
Member

woop commented Jan 3, 2019

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.

@tims
Copy link
Contributor

tims commented Jan 3, 2019

@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.

@tims
Copy link
Contributor

tims commented Jan 9, 2019

Can we close this issue now?

@tims
Copy link
Contributor

tims commented Jan 9, 2019

Closing, but reopen if I missed something.

@tims tims closed this as completed Jan 9, 2019
@pradithya
Copy link
Collaborator Author

Reopening the issue, core is still checking for presence of warehouse store. I am crafting a PR

@pradithya
Copy link
Collaborator Author

I am still having this error when trying to ingest data for feature having no warehouse store.
The feature spec looks like as follow:

id: "myentity.minute.feature_1"
name: "feature_1"
owner: "[email protected]"
description: "Test feature"
uri: "NA"
granularity: "MINUTE"
valueType: "DOUBLE"
entity: "myentity"
dataStores: 
  serving: 
    id: "REDIS1"

Exception:

Optional[Exception in thread "main" java.lang.IllegalArgumentException: no write transforms found
        at com.google.common.base.Preconditions.checkArgument(Preconditions.java:141)
        at feast.ingestion.transform.SplitOutputByStore.expand(SplitOutputByStore.java:57)
        at feast.ingestion.transform.SplitOutputByStore.expand(SplitOutputByStore.java:45)
        at org.apache.beam.sdk.Pipeline.applyInternal(Pipeline.java:537)
        at org.apache.beam.sdk.Pipeline.applyTransform(Pipeline.java:488)
        at feast.ingestion.values.PFeatureRows.apply(PFeatureRows.java:109)
        at feast.ingestion.transform.WarehouseStoreTransform.expand(WarehouseStoreTransform.java:45)
        at feast.ingestion.transform.WarehouseStoreTransform.expand(WarehouseStoreTransform.java:30)
        at org.apache.beam.sdk.Pipeline.applyInternal(Pipeline.java:537)
        at org.apache.beam.sdk.Pipeline.applyTransform(Pipeline.java:488)
        at feast.ingestion.values.PFeatureRows.apply(PFeatureRows.java:109)
        at feast.ingestion.ImportJob.expand(ImportJob.java:184)
        at feast.ingestion.ImportJob.mainWithResult(ImportJob.java:126)
        at feast.ingestion.ImportJob.main(ImportJob.java:110)]

@pradithya
Copy link
Collaborator Author

pradithya commented Jan 18, 2019

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

gitbook-com bot pushed a commit that referenced this issue Nov 25, 2021
dmartinol pushed a commit to dmartinol/feast that referenced this issue May 27, 2024
Initial skeleton of unit test for offline server
tsisodia10 pushed a commit to tsisodia10/feast that referenced this issue Jul 23, 2024
# This is the 1st commit message:

chore: Bump ws from 7.5.6 to 7.5.10 in /ui (feast-dev#4292)


# This is the commit message feast-dev#2:

Remove metric checks

Signed-off-by: Twinkll Sisodia <[email protected]>

# This is the commit message feast-dev#3:

Draft different md file

# This is the commit message feast-dev#4:

Refine opentelemetry.md

# This is the commit message feast-dev#5:

Refine opentelemetry.md

# This is the commit message feast-dev#6:

Refine opentelemetry.md

# This is the commit message feast-dev#7:

Refine opentelemetry.md
tsisodia10 pushed a commit to tsisodia10/feast that referenced this issue Jul 23, 2024
# This is the 1st commit message:

chore: Bump ws from 7.5.6 to 7.5.10 in /ui (feast-dev#4292)


# This is the commit message feast-dev#2:

Remove metric checks

Signed-off-by: Twinkll Sisodia <[email protected]>

# This is the commit message feast-dev#3:

Draft different md file

# This is the commit message feast-dev#4:

Refine opentelemetry.md

# This is the commit message feast-dev#5:

Refine opentelemetry.md

# This is the commit message feast-dev#6:

Refine opentelemetry.md

# This is the commit message feast-dev#7:

Refine opentelemetry.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants