search

feast-dev / feast

The Open Source Feature Store for Machine Learning
https://feast.dev
Apache License 2.0
5.53k stars 991 forks source link

Error Store should not require a storage spec #27

Closed zhilingc closed 5 years ago

zhilingc commented 5 years ago

The error store should be defined on deployment (of core), and should not require a spec.

tims commented 5 years ago

Can you clarify how would you like to tell ingestion where to store the errors?

zhilingc commented 5 years ago

Currently my plan is to pass it in as 2 separate options: --errorsStoreType: one of [file.json, stderr, stdout] - type of errors store to write to --errorsStoreOptions: map of options for the errors store

These would be defined in the config for core feast deployment

is this ok @tims?

tims commented 5 years ago

That looks suspiciously like a storage spec :)

If we're committed to just files and logs.. we could just log to stderr by default if a file path isn't provided?

I imagine we might want avro or parquet too though... hmm.

Would errorsStoreOptions be comma separated kvs? eg. x=1,y=2

zhilingc commented 5 years ago

Sorry, I realised the issue was poorly worded: I meant we should not give error stores the same treatment as other storage specs currently.

The fact that it (1) has to be registered (2) can be retrieved together with all other specs and (3) can be written to as a regular warehouse store by ingestion jobs is what i want to change.

At the same time it would be nice to retain the flexibility to add other potential error store formats in the future, which is why I've split its definition into type and options.

Would errorsStoreOptions be comma separated kvs? eg. x=1,y=2

I was thinking json e.g. {"path":"/path/to/file",...} maybe, but I'm not sure...

tims commented 5 years ago

I think json is ok, as long as it doesn't cause any shell escaping issues for the core api?

reviewing the rest now