Add `to_bigquery()` function to BigQueryRetrievalJob #1634

vtao2 · 2021-06-10T00:42:54Z

What this PR does / why we need it:
This PR adds a to_bigquery() function to the BigQueryRetrievalJob class so that get_historical_features() can scale. This way, instead of writing to a dataframe (resulting in a Response too large to return error), we can write to a temporary destination table in BigQuery.

Without this, get_historical_features() does not scale. It is unable to return a large dataset (eg: ~5B rows) as this data is too big for dataframes.

Which issue(s) this PR fixes:

Fixes #1637

Does this PR introduce a user-facing change?:

Adding a `to_bigquery()` method to BigQueryRetrievalJob

feast-ci-bot · 2021-06-10T00:43:04Z

Hi @vtao2. Thanks for your PR.

I'm waiting for a feast-dev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mavysavydav · 2021-06-11T16:06:20Z

Could we add a parameter for dry run in the method? QueryJobConfig has a dry_run option

MattDelac · 2021-06-14T18:39:59Z

Could we add a parameter for dry run in the method? QueryJobConfig has a dry_run option

Hi @mavysavydav ,

What would be the dry-run used for ? When would that be used with Feast CLI commands ?
This sounds like out of scope for this PR no ?

vtao2 · 2021-06-15T12:39:24Z

/retest

MattDelac · 2021-06-15T12:46:00Z

sdk/python/feast/infra/offline_stores/bigquery.py

+            print(
+                "This query will process {} bytes.".format(bq_job.total_bytes_processed)
+            )
+            return path


Should we just return nothing when we asked for a dry_run ? I don't understand the utility of the path if the table never been created. What I am missing 🤔 ?

if dry_run: return ... return path

It just means that this function will output an Optional[str]

def to_bigquery(self, dry_run=False) -> Optional[str]:

Good point :) changed :D

MattDelac · 2021-06-15T12:47:43Z

sdk/python/tests/test_historical_retrieval.py

@@ -441,6 +441,23 @@ def test_historical_features_from_bigquery_sources(
            ],
        )

+        # Just a dry run, should not create table
+        bq_dry_run = job_from_sql.to_bigquery(dry_run=True)
+        with pytest.raises(Exception):


Do you have access to a more accurate exception like TableNotFound ?

actually, since we won't return the path, can just test that nothing is returned when calling dry run :)

changed it to reflect that

MattDelac

LGTM

vtao2 · 2021-06-15T13:37:15Z

/retest

codecov-commenter · 2021-06-15T15:04:49Z

Codecov Report

Merging #1634 (d2090f2) into master (b32e766) will increase coverage by 0.14%.
The diff coverage is 96.87%.

@@            Coverage Diff             @@
##           master    #1634      +/-   ##
==========================================
+ Coverage   83.39%   83.54%   +0.14%     
==========================================
  Files          71       71              
  Lines        6113     6173      +60     
==========================================
+ Hits         5098     5157      +59     
- Misses       1015     1016       +1

Flag	Coverage Δ
integrationtests	`83.46% <96.87%> (+0.14%)`	⬆️
unittests	`76.25% <15.62%> (-0.21%)`	⬇️

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

Impacted Files	Coverage Δ
sdk/python/feast/infra/offline_stores/bigquery.py	`86.45% <95.65%> (+0.99%)`	⬆️
sdk/python/tests/test_historical_retrieval.py	`98.98% <100.00%> (+0.04%)`	⬆️
sdk/python/tests/test_online_retrieval.py	`100.00% <0.00%> (ø)`
sdk/python/feast/online_response.py	`97.43% <0.00%> (+0.21%)`	⬆️

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 b32e766...d2090f2. Read the comment docs.

Signed-off-by: Vivian Tao <[email protected]>

woop · 2021-06-15T15:53:16Z

@vtao2 Just a small nitpick, but I noticed we're deleting our commits and force pushing the same commit message. This makes it impossible to see the history of changes and also leads to broken links which makes reviews hard, for example https://github.com/feast-dev/feast/pull/1634/files/6b6398530c69f221629b837225be2870fde75263..4cb2f010de767e8d1aef827c0446a0b968baded9

Signed-off-by: Vivian Tao <[email protected]>

sdk/python/feast/infra/offline_stores/bigquery.py

Signed-off-by: Vivian Tao <[email protected]>

sdk/python/setup.py

Signed-off-by: Vivian Tao <[email protected]>

feast-ci-bot · 2021-06-16T19:22:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MattDelac, vtao2, 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:

~~OWNERS~~ [woop]

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

woop · 2021-06-16T19:22:47Z

/lgtm

* Add to_bigquery() function for bq retrieval job Signed-off-by: Vivian Tao <[email protected]> * Using tenacity for retries Signed-off-by: Vivian Tao <[email protected]> * Refactoring to_biquery function Signed-off-by: Vivian Tao <[email protected]> * Adding tenacity dependency and changing temp table prefix to historical Signed-off-by: Vivian Tao <[email protected]> * Use self.client instead of creating a new client Signed-off-by: Vivian Tao <[email protected]> * pin tenacity to major version Signed-off-by: Vivian Tao <[email protected]> * Tenacity dependency range Signed-off-by: Vivian Tao <[email protected]>

feast-ci-bot added do-not-merge/work-in-progress do-not-merge/release-note-label-needed needs-kind labels Jun 10, 2021

feast-ci-bot added needs-ok-to-test size/XS labels Jun 10, 2021

codyjlin mentioned this pull request Jun 11, 2021

Add to_bq() to BigQueryRetrievalJob #1637

Closed

feast-ci-bot added size/S and removed size/XS labels Jun 14, 2021

vtao2 changed the title ~~Vt/to bigquery function~~ Add to_bigquery() function to BigQueryRetrievalJob Jun 14, 2021

feast-ci-bot added release-note and removed do-not-merge/release-note-label-needed labels Jun 14, 2021

vtao2 force-pushed the vt/to_bigquery_function branch from 8e58ec6 to 6508cf2 Compare June 14, 2021 20:07

feast-ci-bot added size/XXL and removed size/S labels Jun 14, 2021

vtao2 force-pushed the vt/to_bigquery_function branch from 6508cf2 to 2f2a5e4 Compare June 14, 2021 20:08

feast-ci-bot added size/S and removed size/XXL labels Jun 14, 2021

vtao2 force-pushed the vt/to_bigquery_function branch from 2f2a5e4 to e5c80b1 Compare June 14, 2021 20:12

feast-ci-bot added size/XXL and removed size/S labels Jun 14, 2021

vtao2 force-pushed the vt/to_bigquery_function branch from e5c80b1 to a5cccd4 Compare June 14, 2021 20:14

feast-ci-bot added size/M and removed size/XXL labels Jun 14, 2021

vtao2 force-pushed the vt/to_bigquery_function branch from a5cccd4 to d6619e7 Compare June 14, 2021 20:18

feast-ci-bot added size/XXL and removed size/M labels Jun 14, 2021

vtao2 force-pushed the vt/to_bigquery_function branch from d6619e7 to 9eedfd1 Compare June 14, 2021 20:21

vtao2 force-pushed the vt/to_bigquery_function branch from 8053848 to 5da2b78 Compare June 15, 2021 12:30

MattDelac reviewed Jun 15, 2021

View reviewed changes

vtao2 force-pushed the vt/to_bigquery_function branch from 5da2b78 to 005e152 Compare June 15, 2021 12:56

MattDelac approved these changes Jun 15, 2021

View reviewed changes

vtao2 force-pushed the vt/to_bigquery_function branch from 005e152 to a883fe6 Compare June 15, 2021 13:12

vtao2 force-pushed the vt/to_bigquery_function branch from a883fe6 to 6b63985 Compare June 15, 2021 13:39

Add to_bigquery() function for bq retrieval job

4cb2f01

Signed-off-by: Vivian Tao <[email protected]>

vtao2 force-pushed the vt/to_bigquery_function branch from 6b63985 to 4cb2f01 Compare June 15, 2021 15:44

vtao2 added 2 commits June 16, 2021 09:13

Using tenacity for retries

e49ce8f

Signed-off-by: Vivian Tao <[email protected]>

Refactoring to_biquery function

c45765c

Signed-off-by: Vivian Tao <[email protected]>

woop reviewed Jun 16, 2021

View reviewed changes

sdk/python/feast/infra/offline_stores/bigquery.py Outdated Show resolved Hide resolved

woop reviewed Jun 16, 2021

View reviewed changes

sdk/python/feast/infra/offline_stores/bigquery.py Outdated Show resolved Hide resolved

vtao2 added 3 commits June 16, 2021 14:00

Adding tenacity dependency and changing temp table prefix to historical

5e5d470

Signed-off-by: Vivian Tao <[email protected]>

Use self.client instead of creating a new client

d7717ef

Signed-off-by: Vivian Tao <[email protected]>

pin tenacity to major version

0ef414d

Signed-off-by: Vivian Tao <[email protected]>

woop reviewed Jun 16, 2021

View reviewed changes

sdk/python/setup.py Outdated Show resolved Hide resolved

Tenacity dependency range

d2090f2

Signed-off-by: Vivian Tao <[email protected]>

woop approved these changes Jun 16, 2021

View reviewed changes

feast-ci-bot added the approved label Jun 16, 2021

feast-ci-bot assigned woop Jun 16, 2021

feast-ci-bot added the lgtm label Jun 16, 2021

feast-ci-bot merged commit 3c46a4d into feast-dev:master Jun 16, 2021

codyjlin mentioned this pull request Jun 22, 2021

Provide the user with more options for setting the to_bigquery config #1661

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `to_bigquery()` function to BigQueryRetrievalJob #1634

Add `to_bigquery()` function to BigQueryRetrievalJob #1634

vtao2 commented Jun 10, 2021 •

edited

Loading

feast-ci-bot commented Jun 10, 2021

mavysavydav commented Jun 11, 2021

MattDelac commented Jun 14, 2021

vtao2 commented Jun 15, 2021

MattDelac Jun 15, 2021

vtao2 Jun 15, 2021

MattDelac Jun 15, 2021

vtao2 Jun 15, 2021

vtao2 Jun 15, 2021

MattDelac left a comment

vtao2 commented Jun 15, 2021

codecov-commenter commented Jun 15, 2021 •

edited

Loading

woop commented Jun 15, 2021 •

edited

Loading

feast-ci-bot commented Jun 16, 2021

woop commented Jun 16, 2021

Add to_bigquery() function to BigQueryRetrievalJob #1634

Add to_bigquery() function to BigQueryRetrievalJob #1634

Conversation

vtao2 commented Jun 10, 2021 • edited Loading

feast-ci-bot commented Jun 10, 2021

mavysavydav commented Jun 11, 2021

MattDelac commented Jun 14, 2021

vtao2 commented Jun 15, 2021

MattDelac Jun 15, 2021

Choose a reason for hiding this comment

vtao2 Jun 15, 2021

Choose a reason for hiding this comment

MattDelac Jun 15, 2021

Choose a reason for hiding this comment

vtao2 Jun 15, 2021

Choose a reason for hiding this comment

vtao2 Jun 15, 2021

Choose a reason for hiding this comment

MattDelac left a comment

Choose a reason for hiding this comment

vtao2 commented Jun 15, 2021

codecov-commenter commented Jun 15, 2021 • edited Loading

Codecov Report

woop commented Jun 15, 2021 • edited Loading

feast-ci-bot commented Jun 16, 2021

woop commented Jun 16, 2021

Add `to_bigquery()` function to BigQueryRetrievalJob #1634

Add `to_bigquery()` function to BigQueryRetrievalJob #1634

vtao2 commented Jun 10, 2021 •

edited

Loading

codecov-commenter commented Jun 15, 2021 •

edited

Loading

woop commented Jun 15, 2021 •

edited

Loading