Description

nanohanno

opened on Oct 29, 2024

Issue body actions

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

feast/sdk/python/feast/infra/registry/registry.py

Line 232 in 4a6b663

self.commit()

which was added in #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

• Version: v0.41.3
• Platform: MacOs
• Subsystem:

Possible Solution

Activity

1. nanohanno

   added

   kind/bug

   priority/p2

   on Oct 29, 2024
2. franciscojavierarceo commented on Oct 29, 2024

   

   franciscojavierarceo

   on Oct 29, 2024

   Member

   More actions

   @EXPEbdodla @tokoko @HaoXuAI @dmartinol
3. dmartinol commented on Oct 29, 2024

   

   dmartinol

   on Oct 29, 2024

   Contributor

   More actions

   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.

   Reacted by nanohanno
4. EXPEbdodla commented on Oct 29, 2024

   

   EXPEbdodla

   on Oct 29, 2024

   Contributor

   More actions

   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?
5. dmartinol commented on Oct 29, 2024

   

   dmartinol

   on Oct 29, 2024

   Contributor

   More actions

   Is there any way to add a test for this?

   I would mock update_registry_proto and raise the error.

   Reacted by Bhargav Dodla
6. franciscojavierarceo commented on Oct 30, 2024

   

   franciscojavierarceo

   on Oct 30, 2024

   Member

   More actions

   @EXPEbdodla do you mind you cutting a PR? 🙏
7. EXPEbdodla commented on Oct 30, 2024

   

   EXPEbdodla

   on Oct 30, 2024

   Contributor

   More actions

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

   closed this as completedin #4719on Oct 31, 2024
9. nanohanno commented on Nov 4, 2024

   

   nanohanno

   on Nov 4, 2024

   ContributorAuthor

   More actions

   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?

Issue actions