Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor data source classes to fix import issues #1723

Merged
merged 3 commits into from
Jul 20, 2021

Conversation

achals
Copy link
Member

@achals achals commented Jul 19, 2021

Signed-off-by: Achal Shah achals@gmail.com

What this PR does / why we need it:

By keeping the Source classes in the same files as the offline store implementations, we can't import the sources without the corresponding libraries installed. This is a terrible experience. This PR moves the classes to a different file, so they can be imported for just the declarative use cases and not require boto or gcloud stuff to be installed.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

NONE

Signed-off-by: Achal Shah <achals@gmail.com>
Signed-off-by: Achal Shah <achals@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Jul 19, 2021

Codecov Report

Merging #1723 (75b09d4) into master (8cfe914) will decrease coverage by 0.00%.
The diff coverage is 77.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1723      +/-   ##
==========================================
- Coverage   84.47%   84.46%   -0.01%     
==========================================
  Files          79       82       +3     
  Lines        7071     7093      +22     
==========================================
+ Hits         5973     5991      +18     
- Misses       1098     1102       +4     
Flag Coverage Δ
integrationtests 84.39% <77.90%> (-0.01%) ⬇️
unittests 69.49% <58.42%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
sdk/python/feast/data_source.py 69.71% <45.45%> (-0.79%) ⬇️
...thon/feast/infra/offline_stores/redshift_source.py 67.81% <67.81%> (ø)
...thon/feast/infra/offline_stores/bigquery_source.py 80.68% <80.68%> (ø)
...k/python/feast/infra/offline_stores/file_source.py 88.73% <88.73%> (ø)
sdk/python/feast/__init__.py 90.00% <100.00%> (ø)
sdk/python/feast/infra/offline_stores/bigquery.py 81.10% <100.00%> (+0.50%) ⬆️
sdk/python/feast/infra/offline_stores/file.py 97.05% <100.00%> (+3.56%) ⬆️
sdk/python/feast/infra/offline_stores/redshift.py 91.66% <100.00%> (+14.74%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8cfe914...75b09d4. Read the comment docs.

@achals
Copy link
Member Author

achals commented Jul 19, 2021

/restest

@woop
Copy link
Member

woop commented Jul 19, 2021

Why do we need __init__ in sdk?

@achals
Copy link
Member Author

achals commented Jul 19, 2021

Why do we need __init__ in sdk?

we don't, intellij added it. removing

Signed-off-by: Achal Shah <achals@gmail.com>
from feast.registry import Registry
from feast.repo_config import FeastConfigBaseModel, RepoConfig
from feast.value_type import ValueType

from .bigquery_source import BigQuerySource
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import is suspect. Should it be feast.bigquery_source?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's a relative import - should work fine (tests are happy too)

Copy link
Member

@woop woop Jul 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember where this has caused problems for me, but I distinctly remember always having to make all imports absolute. Somewhere I ran into problems with relative imports. I don't think there are any relative imports in the code base, except the __init__.py files.

I'm fine leaving it as is and seeing if a problem pops up though.

@woop
Copy link
Member

woop commented Jul 20, 2021

/lgtm

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: achals, woop

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:

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

@feast-ci-bot feast-ci-bot merged commit 8b3a97a into feast-dev:master Jul 20, 2021
@achals achals deleted the achal/source-refactor branch July 20, 2021 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants