FeatureStore instantiation fails after update to 0.41.3

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

@EXPEbdodla

@EXPEbdodla @tokoko @HaoXuAI @dmartinol

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.

@dmartinol

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?

Is there any way to add a test for this?

I would mock update_registry_proto and raise the error.

@EXPEbdodla

@EXPEbdodla do you mind you cutting a PR? 🙏

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

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?

Navigation Menu

Repository navigation

FeatureStore instantiation fails after update to 0.41.3 #4715

Expected Behavior

Current Behavior

Steps to reproduce

Specifications

Possible Solution

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

FeatureStore instantiation fails after update to 0.41.3 #4715

Description

Expected Behavior

Current Behavior

Steps to reproduce

Specifications

Possible Solution

Activity

franciscojavierarceo commented on Oct 29, 2024

dmartinol commented on Oct 29, 2024

EXPEbdodla commented on Oct 29, 2024

dmartinol commented on Oct 29, 2024

franciscojavierarceo commented on Oct 30, 2024

EXPEbdodla commented on Oct 30, 2024

nanohanno commented on Nov 4, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions