fix: Fix bug with no SqlRegistryConfig class #3586

davidschuler-8451 · 2023-04-05T19:23:28Z

What this PR does / why we need it:
Fixes bug with using Sql Registry as described in #3544

Which issue(s) this PR fixes:

sudohainguyen · 2023-04-06T10:23:22Z

hey @davidschuler-8451 , to simplify codebase, I think SqlRegistryConfig should be placed in sql.py instead, and it should inherit RegistryConfig

I'm also working on this one, you can see my commit here but feel free to apply to yours

davidschuler-8451 · 2023-04-06T12:20:44Z

hey @davidschuler-8451 , to simplify codebase, I think SqlRegistryConfig should be placed in sql.py instead, and it should inherit RegistryConfig

I'm also working on this one, you can see my commit here but feel free to apply to yours

thank you for that feedback! I went ahead and made those changes. Doesn't matter to me if your branch or mine gets merged in, just want to get this fixed because it's blocking some progress for us. Thanks!

Signed-off-by: davidschuler-8451 <david.schuler@8451.com>

sudohainguyen · 2023-04-06T12:37:11Z

ah your commits are required DCO to be ready for review, https://github.com/feast-dev/feast/pull/3586/checks?check_run_id=12566589779

davidschuler-8451 · 2023-04-06T12:38:58Z

ah your commits are required DCO to be ready for review, https://github.com/feast-dev/feast/pull/3586/checks?check_run_id=12566589779

yep - think i just fixed it. sorry about that

sudohainguyen · 2023-04-06T13:33:08Z

💯

adchia

/lgtm

feast-ci-bot · 2023-04-06T18:44:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adchia, davidschuler-8451

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [adchia]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

davidschuler-8451 · 2023-04-07T15:25:32Z

Sorry about the linter issue - i have my local env setup now (took a lot of trial and error with an M1 mac) so I can run the linting steps locally.

Might be a couple of ways to solve this:

we could let the SqlRegistry accept either a RegistryConfig or a SqlRegistryConfig like this. Doesn't feel like the cleanest option but it makes the linter happy
registry_config: Union[Optional[SqlRegistryConfig], Optional[RegistryConfig]],
convert the registry_config to an instance of SqlRegistryConfig here? https://github.com/feast-dev/feast/blob/master/sdk/python/feast/infra/registry/registry.py#L175

There may be other ways to do this as well, so I am open to suggestions. thank you!

sudohainguyen · 2023-04-08T03:36:29Z

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

 class SqlRegistry(BaseRegistry):
    def __init__(
        self,
-        registry_config: Optional[RegistryConfig],
+        registry_config: Optional[SqlRegistryConfig],


hey @davidschuler-8451 I have another idea, let's make this one Optional[Union[RegistryConfig, SqlRegistryConfig]]

this should work as a charm

Signed-off-by: davidschuler-8451 <david.schuler@8451.com>

davidschuler-8451 · 2023-04-10T12:41:39Z

@sudohainguyen & @adchia - fixed the issue with the build pipeline and things are looking good. Please review whenever you get a chance. thank you!

adchia · 2023-04-11T16:29:58Z

/lgtm

davidschuler-8451 · 2023-04-13T17:10:59Z

thanks for the review @adchia! This is my first time opening a PR here - anything else I need to do to get this merged and released? Happy to help however necessary!

nathananneken-8451 · 2023-04-18T15:15:04Z

thanks for the review @adchia! This is my first time opening a PR here - anything else I need to do to get this merged and released? Happy to help however necessary!

Anyone have any insight on what next steps are for getting this PR merged? Really appreciate any guidance and assistance!

sudohainguyen · 2023-04-18T15:16:41Z

lint-python action getting stuck somehow, that's why this is not automatically merged by the bot

davidschuler-8451 · 2023-04-18T15:21:08Z

lint-python action getting stuck somehow, that's why this is not automatically merged by the bot

Yeah, the action step says it's waiting on approval from a maintainer even though we already have that. Not sure if any of the @adchia or any of the other maintainers could retrigger that build step? Thanks again for the help

davidschuler-8451 · 2023-04-21T13:55:57Z

hi @achals @felixwang9817 @kevjumba is anyone able to restart the linter action step so this can get merged in? This bug is affecting multiple teams and blocking some progress for us. See #3590, #3544. thank you!

# [0.31.0](v0.30.0...v0.31.0) (2023-04-21) ### Bug Fixes * Add Stream Feature Views to helper that collect Feature View names ([#3582](#3582)) ([7854f63](7854f63)) * Add StreamFeatureViewSpec to FeastObjectSpecProto convenience type ([#3550](#3550)) ([3cefd6c](3cefd6c)) * Batch Snowflake materialization queries to obey Snowpark 100 fea… ([#3406](#3406)) ([f9862b5](f9862b5)) * Bytewax materializer security context ([#3573](#3573)) ([6794338](6794338)) * **cI:** Install coreutils in mac github workers for smoke test ([#3563](#3563)) ([e7421c1](e7421c1)) * Fix bug with no SqlRegistryConfig class ([#3586](#3586)) ([6dc1368](6dc1368)) * Fix Snowflake template ([#3584](#3584)) ([6c09c39](6c09c39)) * Make snowflake to remote tables temporary ([#3588](#3588)) ([ad48146](ad48146)) * Remove snowflake source warehouse tech debt ([#3422](#3422)) ([7da0580](7da0580)) * Snowflake remote storage ([#3574](#3574)) ([f8d3890](f8d3890)) * Support param timeout when persisting ([#3593](#3593)) ([01a98f0](01a98f0)) * Use pyarrow in a way that works across versions ([#3562](#3562)) ([1289f3f](1289f3f)) * Wrap the bigquery table name with backtick. ([#3577](#3577)) ([09f0e7e](09f0e7e)) ### Features * Add AWS Redshift Serverless support ([#3595](#3595)) ([58ce148](58ce148)) * Add Hazelcast as an online store ([#3523](#3523)) ([b05d50b](b05d50b)) * Cache Bigtable client ([#3602](#3602)) ([b27472f](b27472f)) * Relax aws extras requirements ([#3585](#3585)) ([7e77382](7e77382)) * Show bigquery datasource table and query on UI ([#3600](#3600)) ([58d63f7](58d63f7)) * Update snowflake offline store job output formats -- added arrow ([#3589](#3589)) ([be3e349](be3e349))

* adding SqlRegistryConfig class Signed-off-by: davidschuler-8451 <david.schuler@8451.com> * refactor: move SqlRegistryConfig class to sql.py Signed-off-by: davidschuler-8451 <david.schuler@8451.com> * enabling SqlRegistry to accept RegistryConfig or SqlRegistryConfig Signed-off-by: davidschuler-8451 <david.schuler@8451.com> --------- Signed-off-by: davidschuler-8451 <david.schuler@8451.com>

# [0.31.0](feast-dev/feast@v0.30.0...v0.31.0) (2023-04-21) ### Bug Fixes * Add Stream Feature Views to helper that collect Feature View names ([feast-dev#3582](feast-dev#3582)) ([7854f63](feast-dev@7854f63)) * Add StreamFeatureViewSpec to FeastObjectSpecProto convenience type ([feast-dev#3550](feast-dev#3550)) ([3cefd6c](feast-dev@3cefd6c)) * Batch Snowflake materialization queries to obey Snowpark 100 fea… ([feast-dev#3406](feast-dev#3406)) ([f9862b5](feast-dev@f9862b5)) * Bytewax materializer security context ([feast-dev#3573](feast-dev#3573)) ([6794338](feast-dev@6794338)) * **cI:** Install coreutils in mac github workers for smoke test ([feast-dev#3563](feast-dev#3563)) ([e7421c1](feast-dev@e7421c1)) * Fix bug with no SqlRegistryConfig class ([feast-dev#3586](feast-dev#3586)) ([6dc1368](feast-dev@6dc1368)) * Fix Snowflake template ([feast-dev#3584](feast-dev#3584)) ([6c09c39](feast-dev@6c09c39)) * Make snowflake to remote tables temporary ([feast-dev#3588](feast-dev#3588)) ([ad48146](feast-dev@ad48146)) * Remove snowflake source warehouse tech debt ([feast-dev#3422](feast-dev#3422)) ([7da0580](feast-dev@7da0580)) * Snowflake remote storage ([feast-dev#3574](feast-dev#3574)) ([f8d3890](feast-dev@f8d3890)) * Support param timeout when persisting ([feast-dev#3593](feast-dev#3593)) ([01a98f0](feast-dev@01a98f0)) * Use pyarrow in a way that works across versions ([feast-dev#3562](feast-dev#3562)) ([1289f3f](feast-dev@1289f3f)) * Wrap the bigquery table name with backtick. ([feast-dev#3577](feast-dev#3577)) ([09f0e7e](feast-dev@09f0e7e)) ### Features * Add AWS Redshift Serverless support ([feast-dev#3595](feast-dev#3595)) ([58ce148](feast-dev@58ce148)) * Add Hazelcast as an online store ([feast-dev#3523](feast-dev#3523)) ([b05d50b](feast-dev@b05d50b)) * Cache Bigtable client ([feast-dev#3602](feast-dev#3602)) ([b27472f](feast-dev@b27472f)) * Relax aws extras requirements ([feast-dev#3585](feast-dev#3585)) ([7e77382](feast-dev@7e77382)) * Show bigquery datasource table and query on UI ([feast-dev#3600](feast-dev#3600)) ([58d63f7](feast-dev@58d63f7)) * Update snowflake offline store job output formats -- added arrow ([feast-dev#3589](feast-dev#3589)) ([be3e349](feast-dev@be3e349)) Signed-off-by: zerafachris PERSONAL <zerafachris@gmail.com>

feast-ci-bot added the size/S label Apr 5, 2023

This was referenced Apr 5, 2023

SqlRegistryConfig does not exist #3544

Closed

Could not import module 'feast.infra.offline_stores.snowflake' #3580

Closed

davidschuler-8451 added 2 commits April 6, 2023 08:36

adding SqlRegistryConfig class

38de02f

Signed-off-by: davidschuler-8451 <david.schuler@8451.com>

refactor: move SqlRegistryConfig class to sql.py

4c1e16f

Signed-off-by: davidschuler-8451 <david.schuler@8451.com>

adchia approved these changes Apr 6, 2023

View reviewed changes

feast-ci-bot assigned adchia Apr 6, 2023

feast-ci-bot added the lgtm label Apr 6, 2023

feast-ci-bot added the approved label Apr 6, 2023

adchia changed the title ~~fix: SqlRegistryConfig class~~ fix: Fix bug with no SqlRegistryConfig class Apr 6, 2023

sudohainguyen reviewed Apr 8, 2023

View reviewed changes

enabling SqlRegistry to accept RegistryConfig or SqlRegistryConfig

e7864dc

Signed-off-by: davidschuler-8451 <david.schuler@8451.com>

feast-ci-bot removed the lgtm label Apr 10, 2023

feast-ci-bot added the lgtm label Apr 11, 2023

sudohainguyen mentioned this pull request Apr 19, 2023

FeastClassImportError: Could not import class 'SqlRegistryConfig' from module 'feast.infra.registry.sql' #3590

Closed

adchia linked an issue Apr 21, 2023 that may be closed by this pull request

FeastClassImportError: Could not import class 'SqlRegistryConfig' from module 'feast.infra.registry.sql' #3590

Closed

adchia merged commit 6dc1368 into feast-dev:master Apr 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fix bug with no SqlRegistryConfig class #3586

fix: Fix bug with no SqlRegistryConfig class #3586

davidschuler-8451 commented Apr 5, 2023

sudohainguyen commented Apr 6, 2023 •

edited

Loading

davidschuler-8451 commented Apr 6, 2023

sudohainguyen commented Apr 6, 2023

davidschuler-8451 commented Apr 6, 2023

sudohainguyen commented Apr 6, 2023

adchia left a comment

feast-ci-bot commented Apr 6, 2023

davidschuler-8451 commented Apr 7, 2023

sudohainguyen Apr 8, 2023 •

edited

Loading

davidschuler-8451 commented Apr 10, 2023

adchia commented Apr 11, 2023

davidschuler-8451 commented Apr 13, 2023

nathananneken-8451 commented Apr 18, 2023

sudohainguyen commented Apr 18, 2023

davidschuler-8451 commented Apr 18, 2023

davidschuler-8451 commented Apr 21, 2023

fix: Fix bug with no SqlRegistryConfig class #3586

fix: Fix bug with no SqlRegistryConfig class #3586

Conversation

davidschuler-8451 commented Apr 5, 2023

sudohainguyen commented Apr 6, 2023 • edited Loading

davidschuler-8451 commented Apr 6, 2023

sudohainguyen commented Apr 6, 2023

davidschuler-8451 commented Apr 6, 2023

sudohainguyen commented Apr 6, 2023

adchia left a comment

Choose a reason for hiding this comment

feast-ci-bot commented Apr 6, 2023

davidschuler-8451 commented Apr 7, 2023

sudohainguyen Apr 8, 2023 • edited Loading

Choose a reason for hiding this comment

davidschuler-8451 commented Apr 10, 2023

adchia commented Apr 11, 2023

davidschuler-8451 commented Apr 13, 2023

nathananneken-8451 commented Apr 18, 2023

sudohainguyen commented Apr 18, 2023

davidschuler-8451 commented Apr 18, 2023

davidschuler-8451 commented Apr 21, 2023

sudohainguyen commented Apr 6, 2023 •

edited

Loading

sudohainguyen Apr 8, 2023 •

edited

Loading