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.

Sign up for GitHub

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

Jump to bottom

Improve exception handling, logging, and validation #1477

Merged
merged 11 commits into from
Apr 20, 2021
Merged

Improve exception handling, logging, and validation #1477

merged 11 commits into from
Apr 20, 2021

Conversation

woop
Copy link
Member

@woop woop commented Apr 19, 2021 •
edited
Loading

What this PR does / why we need it:

  • Add better logging to the Apply CLI method
  • Add error message when missing authentication for GCP provider
  • Add proper error message for missing feature views in historical retrieval
  • Add proper error message for credential error for BigQuery client
  • Add proper GCP project missing error for BigQuery client
  • Ensure all apply commands initialize a feature repository
  • Ensure FeatureStore object can be run from outside a repository

Does this PR introduce a user-facing change?:

* Add clearer logging to the Apply CLI method
* Add error message when missing authentication for GCP provider  
* Add proper error message for missing feature views in historical retrieval
* Add proper error message for credential error for BigQuery client
* Add proper GCP project missing error for BigQuery client
* Ensure all apply commands initialize a feature repository
* Ensure FeatureStore object can be run from outside a repository path (non current working dir)

@woop woop requested review from jklegar and tsotnet as code owners April 19, 2021 01:57
@woop woop changed the title [WIP] Improve exception handling and logging Improve exception handling, logging, and validation Apr 19, 2021
@woop
Fix linting
0af71ad 
Signed-off-by: Willem Pienaar <[email protected]>
@woop woop mentioned this pull request Apr 19, 2021
Closed
tsotnet
tsotnet requested changes Apr 20, 2021
View reviewed changes
sdk/python/feast/infra/gcp.py Outdated
else:
return datastore.Client()
except DefaultCredentialsError as e:
assert isinstance(e, DefaultCredentialsError)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this isn't necessary

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, good catch

sdk/python/feast/infra/offline_stores/bigquery.py Outdated
from google.cloud import bigquery

client = bigquery.Client()
client = BigQueryOfflineStore._get_bigquery_client()
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can make this a @classmethod and call cls._get_bigquery_client()

sdk/python/feast/infra/offline_stores/bigquery.py Outdated
@@ -76,14 +98,18 @@ def get_historical_features(
) -> RetrievalJob:
# TODO: Add entity_df validation in order to fail before interacting with BigQuery

client = BigQueryOfflineStore._get_bigquery_client()
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here. Generally if you're accessing class methods, it's better to make the method a classmethod.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've been tainted by Java

Copy link
Collaborator

Choose a reason for hiding this comment

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

lol :D

sdk/python/feast/registry.py Outdated
def _initialize_registry(self):
"""Explicitly forces the initialization of a registry"""

def updater(registry_proto: RegistryProto):
Copy link
Collaborator

Choose a reason for hiding this comment

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

might be better to allow None as a parameter if updater isn't doing anything

sdk/python/tests/test_cli_local.py Show resolved Hide resolved
tsotnet
tsotnet approved these changes Apr 20, 2021
View reviewed changes
Copy link
Collaborator

@tsotnet tsotnet left a comment

Choose a reason for hiding this comment

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

/lgtm

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tsotnet, 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 9ec90f4 into feast-dev:master Apr 20, 2021
@woop woop mentioned this pull request Apr 21, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsotnet tsotnet tsotnet approved these changes

@jklegar jklegar Awaiting requested review from jklegar

Assignees

@tsotnet tsotnet

Labels
approved kind/housekeeping lgtm release-note size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@woop @feast-ci-bot @tsotnet