Repository navigation
Description
Expected Behavior
When specifying "sql" as the registry type via the python SDK, feast uses the appropriate config class.
Current Behavior
When specifying "sql" as the registry type via the python SDK, calling the FeatureStore object results in this error:
feast.errors.FeastClassImportError: Could not import class 'SqlRegistryConfig' from module 'feast.infra.registry.sql'
get_registry_config_from_type builds the class name here: https://github.com/feast-dev/feast/blob/master/sdk/python/feast/repo_config.py#L499 causing the config to look for the SqlRegistryConfig class, which doesn't exist. This is due to the code adding Config to the end of the current registry class, which works when specifying "file" (which uses the RegistryConfig class, which exists), but not for "sql".
Steps to reproduce
Set my_repo_config = RepoConfig(registry=RepoConfig(registry_type="sql")) and then call FeatureStore(config=my_repo_config), which will then result in the error described above.
Specifications
- Version: 0.30.0
- Platform: Ubuntu
- Subsystem:
Possible Solution
Option 1:
Change the REGISTRY_CLASS_FOR_TYPE dict so that "sql" maps to "feast.infra.registry.registry.Registry" instead of "feast.infra.registry.sql.SqlRegistry" - This may also have to be done for "snowflake.registry"
Option 2:
Create the SqlRegistryConfig class in feast.infra.registry.sql. I'm also not seeing a SnowflakeRegistryConfig class in the feast.infra.registry.snowflake module, so that may need to be created as well.
Option 3:
Alter the get_registry_config_from_type function so it builds out the class names differently based on they registry type
https://github.com/feast-dev/feast/blob/master/sdk/python/feast/repo_config.py#L499
Activity
GDegrove commentedon Mar 28, 2023
From what I can see, there's already to type of pattern for loading the configuration class for a type.
Most of the config uses the class name to derive the config, except the feature server that uses a specific ENUM because of the
We do not support custom Feature Serverhttps://github.com/feast-dev/feast/blob/master/sdk/python/feast/repo_config.py#L535
Looking at this, I guess it's safe to say that we should create a
SqlRegistryConfigclass for theSQLRegistryclass, to follow the other design pattern. At first, theSQLRegistryConfigwould be as easy as being an extension of the Registry, then i required it can become more complex.I see that this bug priority is
p2. Does that mean that it will be picked up ASAP?If you are ok with the plan, I think we can open a PR and propose a fix for this issue quite fast (it does seem to be a low-hanging fruit solution, that would work).
Maybe there's other concern that I don't grasp?
Edit: this still exists in version
0.30.2davidschuler-8451 commentedon Apr 5, 2023
hi @GDegrove & @connorwaldman - I attempted a PR for this fix. Confirmed it is behaving as expected on my local tests. could you please take a look and see if this is what you had in mind? #3586
thanks!
connorwaldman commentedon Apr 6, 2023
@davidschuler-8451 Yeah looks great! Thanks!