-
Notifications
You must be signed in to change notification settings - Fork 996
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
chore: Suppress alpha warnings in test. Fix entity serialization in test #3029
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3029 +/- ##
==========================================
+ Coverage 67.72% 77.63% +9.91%
==========================================
Files 167 194 +27
Lines 14679 16109 +1430
==========================================
+ Hits 9941 12507 +2566
+ Misses 4738 3602 -1136
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. |
Signed-off-by: Danny Chiao <danny@tecton.ai>
Signed-off-by: Danny Chiao <danny@tecton.ai>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
@@ -6,7 +6,7 @@ | |||
from pandas.testing import assert_frame_equal as pd_assert_frame_equal | |||
from pytz import utc | |||
|
|||
from feast import FeatureStore, utils | |||
from feast import FeatureService, FeatureStore, utils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally these imports should come from feast.feature_service
, feast.feature_store
, etc.
Also, utils
:(
# TODO(adchia): figure out why entity_key_serialization_version 2 breaks with this test | ||
lambda_environment = construct_test_environment( | ||
lambda_config, None, entity_key_serialization_version=1 | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably becase we need to build and push a new docker image (that uses the new version number). The docker image isn't automatically updated via CI at the moment.
@@ -8,7 +8,7 @@ | |||
import dill | |||
from typeguard import typechecked | |||
|
|||
from feast import utils | |||
from feast import flags_helper, utils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same import comment.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: achals, adchia, kevjumba 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 |
What this PR does / why we need it:
The
make test-python
andmake test-python-integration-local
commands were littered with warnings. This suppresses that to make for a better testing experience.Note: test_lambda fails when switching to entity_key_serialization_version=2 so this temporarily keeps it at 1 for that test