Add Dockerfile for GCP CloudRun FeatureServer #1887

judahrand · 2021-09-20T11:40:32Z

What this PR does / why we need it:
Pretty self explanatory.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

Add Dockerfile for GCP CloudRun FeatureServer.

feast-ci-bot · 2021-09-20T11:40:41Z

Hi @judahrand. 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.

codecov-commenter · 2021-09-20T12:40:33Z

Codecov Report

Merging #1887 (0892133) into master (df724a8) will decrease coverage by 1.50%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1887      +/-   ##
==========================================
- Coverage   81.89%   80.39%   -1.51%     
==========================================
  Files          97       99       +2     
  Lines        7756     9345    +1589     
==========================================
+ Hits         6352     7513    +1161     
- Misses       1404     1832     +428

Flag	Coverage Δ
integrationtests	`75.50% <ø> (+1.44%)`	⬆️
unittests	`59.28% <ø> (ø)`

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

Impacted Files	Coverage Δ
sdk/python/feast/repo_config.py	`78.97% <ø> (ø)`
...fline_store/test_universal_historical_retrieval.py	`75.29% <0.00%> (-24.00%)`	⬇️
...n/feature_repos/universal/data_sources/bigquery.py	`74.57% <0.00%> (-23.05%)`	⬇️
sdk/python/feast/infra/offline_stores/bigquery.py	`64.47% <0.00%> (-15.64%)`	⬇️
sdk/python/feast/infra/offline_stores/redshift.py	`69.37% <0.00%> (-15.63%)`	⬇️
sdk/python/tests/utils/data_source_utils.py	`84.61% <0.00%> (-15.39%)`	⬇️
sdk/python/feast/infra/utils/aws_utils.py	`62.96% <0.00%> (-8.06%)`	⬇️
sdk/python/feast/repo_operations.py	`36.85% <0.00%> (-7.36%)`	⬇️
...python/feast/infra/offline_stores/offline_store.py	`84.12% <0.00%> (-3.38%)`	⬇️
...python/feast/infra/offline_stores/offline_utils.py	`88.46% <0.00%> (-1.76%)`	⬇️
... and 14 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 df724a8...0892133. Read the comment docs.

adchia · 2021-09-20T22:44:11Z

/ok-to-test

felixwang9817 · 2021-09-27T20:38:23Z

Hey @judahrand, thanks for this PR! We're currently working on serverless deployments on AWS Lambda, so a few things will likely be changing over the next couple of weeks (e.g. in the app.py and config.py files you have). Are GCP serverless deployments a blocker for you right now? If not, we'd prefer to delay merging in this PR until the AWS Lambda work has landed, which should be in ~2 weeks.

judahrand · 2021-10-01T12:29:36Z

Works for me! Happy to rework this then if need be.

felixwang9817 · 2021-10-01T18:01:06Z

Great, thanks @judahrand. I'll let you know when the AWS serverless changes have landed.

achals · 2021-10-15T04:29:21Z

sdk/python/feast/infra/feature_servers/gcp_cloudrun/config.py

+    execution_role_name: StrictStr
+    """The execution role for the AWS Lambda function."""


Probably don't need this for gcp?

achals · 2021-10-15T04:29:50Z

sdk/python/feast/infra/feature_servers/gcp_cloudrun/config.py

+    enabled: StrictBool = False
+    """Whether the feature server should be launched."""
+
+    public: StrictBool = True
+    """Whether the endpoint should be publicly accessible."""


Are these used anywhere?

I assume so? They are defined as the required configuration parameters in the AWS Lambda RFC. I assume we'd want to follow the same interface?

achals · 2021-10-15T04:30:28Z

sdk/python/feast/infra/feature_servers/gcp_cloudrun/app.py

+store = FeatureStore(repo_path=str(repo_path.resolve()))
+
+# Create the FastAPI app
+app = get_app(store)


Is the only diff between this and the aws lambda app the fact that that one uses mangum?

Yes, I believe that is the only difference in the app.py file.

Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com>

judahrand · 2021-10-15T08:58:12Z

@achals Failures don't look like my fault

achals · 2021-10-15T16:45:03Z

@achals Failures don't look like my fault

yeah i fixed these (integ test clean up issue). Can you remove the role name field from the gcp config? Should be g2g after that.

judahrand · 2021-10-15T18:39:57Z

@achals Failures don't look like my fault

yeah i fixed these (integ test clean up issue). Can you remove the role name field from the gcp config? Should be g2g after that.

I did, didn't I? 🤔 Do you mean something other than what I removed?

achals · 2021-10-15T18:50:56Z

@achals Failures don't look like my fault

yeah i fixed these (integ test clean up issue). Can you remove the role name field from the gcp config? Should be g2g after that.

I did, didn't I? 🤔 Do you mean something other than what I removed?

Oh oops I was looking at an older version of the PR. You did, my bad.

woop · 2021-10-15T21:17:29Z

Shouldn't dockerfiles be in https://github.com/feast-dev/feast/tree/master/infra/docker?

judahrand · 2021-10-15T21:19:52Z

🤷 I just followed the pattern 😛

The AWS one lives in the equivalent place at the moment.

I kinda think this is okay. Otherwise, the resources for the Feature Server cloud deployments will get spread out and more difficult to work on.

judahrand · 2021-10-18T10:47:00Z

Shouldn't dockerfiles be in https://github.com/feast-dev/feast/tree/master/infra/docker?

Also I think the answer here is no as my understanding is that these Dockerfiles are part of the SDK. There are needed in order to locally build the images and push them to the user's registry.

judahrand · 2021-10-20T08:26:52Z

@achals Do you think this is worth merging and then working on the other bits needed to complete the CloudRun support? (The failed test looks like a timeout)

felixwang9817

/lgtm

feast-ci-bot · 2021-10-20T20:00:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: felixwang9817, judahrand

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~~ [felixwang9817]

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

felixwang9817 · 2021-10-20T20:00:20Z

/ok-to-test

felixwang9817 · 2021-10-20T20:00:23Z

/kind feature

felixwang9817 · 2021-10-20T20:07:56Z

Hey @judahrand, your changes lgtm but for some reason integration tests are failing (doesn't seem to be due to your changes). I'm going to take a look and see if I fix the integration test failures.

Edit: nvm, looks like your changes have been merged.

judahrand · 2021-10-20T20:35:40Z

Hey @judahrand, your changes lgtm but for some reason integration tests are failing (doesn't seem to be due to your changes). I'm going to take a look and see if I fix the integration test failures.

Edit: nvm, looks like your changes have been merged.

It looked like a timeout when I looked briefly - job was cancelled rather than failure iirc.

judahrand requested review from achals, adchia, felixwang9817, tsotnet, woop and a team as code owners September 20, 2021 11:40

feast-ci-bot added do-not-merge/release-note-label-needed needs-kind needs-ok-to-test labels Sep 20, 2021

feast-ci-bot added size/M release-note and removed do-not-merge/release-note-label-needed labels Sep 20, 2021

felixwang9817 self-assigned this Sep 20, 2021

feast-ci-bot added ok-to-test and removed needs-ok-to-test labels Sep 20, 2021

achals reviewed Oct 15, 2021

View reviewed changes

judahrand force-pushed the docker-cloudrun branch from 154f711 to 9c3e1c9 Compare October 15, 2021 08:19

Add gcp_cloudrun Dockerfile and resources

a21d709

Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com>

judahrand force-pushed the docker-cloudrun branch 2 times, most recently from fafdc80 to 041b0e4 Compare October 15, 2021 08:24

judahrand added 2 commits October 15, 2021 09:30

Remove unnecessary config

eff5148

Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com>

Sync CloudRun app.py with AWS Lambda

0892133

Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com>

judahrand force-pushed the docker-cloudrun branch from 041b0e4 to 0892133 Compare October 15, 2021 08:30

felixwang9817 approved these changes Oct 20, 2021

View reviewed changes

feast-ci-bot added the lgtm label Oct 20, 2021

feast-ci-bot added the approved label Oct 20, 2021

feast-ci-bot added kind/feature New feature or request and removed needs-kind labels Oct 20, 2021

feast-ci-bot merged commit 41affbb into feast-dev:master Oct 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Dockerfile for GCP CloudRun FeatureServer #1887

Add Dockerfile for GCP CloudRun FeatureServer #1887

judahrand commented Sep 20, 2021 •

edited

Loading

feast-ci-bot commented Sep 20, 2021

codecov-commenter commented Sep 20, 2021 •

edited

Loading

adchia commented Sep 20, 2021

felixwang9817 commented Sep 27, 2021

judahrand commented Oct 1, 2021

felixwang9817 commented Oct 1, 2021

achals Oct 15, 2021

achals Oct 15, 2021

judahrand Oct 15, 2021 •

edited

Loading

achals Oct 15, 2021

judahrand Oct 15, 2021

judahrand commented Oct 15, 2021

achals commented Oct 15, 2021

judahrand commented Oct 15, 2021

achals commented Oct 15, 2021

woop commented Oct 15, 2021

judahrand commented Oct 15, 2021 •

edited

Loading

judahrand commented Oct 18, 2021

judahrand commented Oct 20, 2021

felixwang9817 left a comment

feast-ci-bot commented Oct 20, 2021

felixwang9817 commented Oct 20, 2021

felixwang9817 commented Oct 20, 2021

felixwang9817 commented Oct 20, 2021 •

edited

Loading

judahrand commented Oct 20, 2021

		execution_role_name: StrictStr
		"""The execution role for the AWS Lambda function."""

Add Dockerfile for GCP CloudRun FeatureServer #1887

Add Dockerfile for GCP CloudRun FeatureServer #1887

Conversation

judahrand commented Sep 20, 2021 • edited Loading

feast-ci-bot commented Sep 20, 2021

codecov-commenter commented Sep 20, 2021 • edited Loading

Codecov Report

adchia commented Sep 20, 2021

felixwang9817 commented Sep 27, 2021

judahrand commented Oct 1, 2021

felixwang9817 commented Oct 1, 2021

achals Oct 15, 2021

Choose a reason for hiding this comment

achals Oct 15, 2021

Choose a reason for hiding this comment

judahrand Oct 15, 2021 • edited Loading

Choose a reason for hiding this comment

achals Oct 15, 2021

Choose a reason for hiding this comment

judahrand Oct 15, 2021

Choose a reason for hiding this comment

judahrand commented Oct 15, 2021

achals commented Oct 15, 2021

judahrand commented Oct 15, 2021

achals commented Oct 15, 2021

woop commented Oct 15, 2021

judahrand commented Oct 15, 2021 • edited Loading

judahrand commented Oct 18, 2021

judahrand commented Oct 20, 2021

felixwang9817 left a comment

Choose a reason for hiding this comment

feast-ci-bot commented Oct 20, 2021

felixwang9817 commented Oct 20, 2021

felixwang9817 commented Oct 20, 2021

felixwang9817 commented Oct 20, 2021 • edited Loading

judahrand commented Oct 20, 2021

judahrand commented Sep 20, 2021 •

edited

Loading

codecov-commenter commented Sep 20, 2021 •

edited

Loading

judahrand Oct 15, 2021 •

edited

Loading

judahrand commented Oct 15, 2021 •

edited

Loading

felixwang9817 commented Oct 20, 2021 •

edited

Loading