Improve GCP exception handling #1561
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
|
/lgtm |
|
New changes are detected. LGTM label has been removed. |
woop
changed the title
| @@ -27,6 +26,14 @@ | |||
| from feast.registry import Registry | |||
| from feast.repo_config import DatastoreOnlineStoreConfig, RepoConfig | |||
|
|
|||
| try: | |||
| from google.auth.exceptions import DefaultCredentialsError | |||
| from google.cloud import bigquery, datastore | |||
There was a problem hiding this comment.
iirc we didn't have google cloud dependencies imported at the top because Feast will try to import them even when the user is not using gcp (e.g. they're just trying things locally) due to this file being imported somewhere else. If you uninstall the google cloud pip packages in your venv and try the quickstart you can confirm whether this happens
There was a problem hiding this comment.
Yea, but I think its cleaner to conditionally import our providers and stores than their subdependencies. Let me test to validate it.
There was a problem hiding this comment.
I think only this change was necessary. Our tests are failing for other reasons
| @@ -20,6 +19,15 @@ | |||
| from feast.registry import Registry | |||
| from feast.repo_config import RepoConfig | |||
|
|
|||
| try: | |||
| from google.auth.exceptions import DefaultCredentialsError | |||
| from google.cloud import bigquery | |||
There was a problem hiding this comment.
here as well, I think (though it may have been just one of gcp.py and bigquery.py where this happened)
Signed-off-by: Willem Pienaar <[email protected]>
Signed-off-by: Willem Pienaar <[email protected]>
Signed-off-by: Willem Pienaar <[email protected]>
Signed-off-by: Willem Pienaar <[email protected]>
Signed-off-by: Willem Pienaar <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #1561 +/- ##
==========================================
- Coverage 83.81% 83.72% -0.09%
==========================================
Files 65 65
Lines 5628 5634 +6
==========================================
Hits 4717 4717
- Misses 911 917 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
* Remove try/catch that fails silently Signed-off-by: Willem Pienaar <[email protected]> * Remove unused import Signed-off-by: Willem Pienaar <[email protected]> * Provided clearer exception handling for GCP dependencies Signed-off-by: Willem Pienaar <[email protected]> * Ensure that GCP dependencies aren't loaded unnecessarily Signed-off-by: Willem Pienaar <[email protected]> * Fix lint Signed-off-by: Willem Pienaar <[email protected]>
Signed-off-by: Willem Pienaar [email protected]
What this PR does / why we need it:
If no
gcloudaccount is specified, then users today are not provided with a useful exception. Instead, the exception is hidden behind a failure due to an anonymous client not having access to their resources.Does this PR introduce a user-facing change?: