search

feast-dev / feast

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

FeatureStore instantiation fails after update to 0.41.3 #4715

Closed nanohanno closed 3 weeks ago

nanohanno commented 3 weeks ago

Expected Behavior

We have started using feast (v0.40.0) to create a feature registry on S3. In a script to fetch data from the offline store the feature store is instantiated with configuration from the default config file. Afterwards, data can be fetched in the script. The registry is supposed to be changed only by dedicated users.

Current Behavior

The script fails with AccessDenied error because it tries to change the registry on S3 but there are no sufficient access credentials configured.

Essentially, feast fails at https://github.com/feast-dev/feast/blob/4a6b663f80bc91d6de35ed2ec428d34811d17a18/sdk/python/feast/infra/registry/registry.py#L232 which was added in https://github.com/feast-dev/feast/pull/4475 .

I do not understand the complete logic but why do we make a commit to the registry when we only want to use the existing registry? Should we have a flag here to disable it in cases where we only want to read the registry, like fetching data or feast plan?

Steps to reproduce

Call fs = FeatureStore() with existing feature store config and S3 registry without configuring PutObject policies for current user.

Specifications

Possible Solution

franciscojavierarceo commented 3 weeks ago

@EXPEbdodla @tokoko @HaoXuAI @dmartinol

dmartinol commented 3 weeks ago

Agree, probably the only one reason to commit is when the registry it's not there, so I would change it to:

            except FileNotFoundError:
                logger.info("Registry file not found. Creating new registry.")
                self.commit()

If the user does not have sufficient permissions, the operation will fail, but this is to be expected.

EXPEbdodla commented 3 weeks ago

I wasn't aware of the use case. We can change to as @dmartinol mentioned. But we are also doing the _sync_feast_metadata_to_projects_table to Projects tables during the __init__(). We may need to control that for reader roles. It won't commit to registry if we change to as mentioned. but its just in cache object. Will be attempted to commit during the registry apply or delete operations. It's definitely not good. But doesn't lead to problems.

Is there any way to add a test for this?

dmartinol commented 3 weeks ago

Is there any way to add a test for this?

I would mock update_registry_proto and raise the error.

franciscojavierarceo commented 3 weeks ago

@EXPEbdodla do you mind you cutting a PR? 🙏

EXPEbdodla commented 3 weeks ago

Added the code fix. I'll add the test case in the next PR to support this use case going further.

nanohanno commented 2 weeks ago

Thanks for the quick response and the fix. Could we have a patch release soon to be able to use the last updates to the library?