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.

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

Fix flaky tests (test_online_store_cleanup & test_feature_get_online_features_types_match) #2276

Merged
merged 4 commits into from
Feb 6, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions sdk/python/feast/type_map.py
Original file line number Diff line number Diff line change
Expand Up @@ -300,9 +300,6 @@ def _python_value_to_proto_value(
"""
# ToDo: make a better sample for type checks (more than one element)
sample = next(filter(_non_empty_value, values), None) # first not empty value
if sample is None:
# all input values are None or empty lists
return [ProtoValue()] * len(values)

# Detect list type and handle separately
if "list" in feast_value_type.name.lower():
Expand All @@ -312,7 +309,9 @@ def _python_value_to_proto_value(
feast_value_type
]

if not all(type(item) in valid_types for item in sample):
if sample is not None and not all(
type(item) in valid_types for item in sample
):
first_invalid = next(
item for item in sample if type(item) not in valid_types
)
Expand All @@ -337,6 +336,10 @@ def _python_value_to_proto_value(

# Handle scalar types below
else:
if sample is None:
# all input values are None
return [ProtoValue()] * len(values)

if feast_value_type == ValueType.UNIX_TIMESTAMP:
int_timestamps = _python_datetime_to_int_timestamp(values)
# ProtoValue does actually accept `np.int_` but the typing complains.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import re
import tempfile
import uuid
from dataclasses import dataclass, field
from dataclasses import dataclass
from datetime import datetime, timedelta
from pathlib import Path
from typing import Any, Dict, List, Optional, Union
Expand Down Expand Up @@ -245,11 +245,8 @@ class Environment:
python_feature_server: bool
worker_id: str

end_date: datetime = field(
default=datetime.utcnow().replace(microsecond=0, second=0, minute=0)
)

def __post_init__(self):
self.end_date = datetime.utcnow().replace(microsecond=0, second=0, minute=0)
self.start_date: datetime = self.end_date - timedelta(days=3)

def get_feature_server_endpoint(self) -> str:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,21 @@
import time
import unittest
from datetime import timedelta
from typing import Any, Dict, List, Union
from typing import Any, Dict, List, Tuple, Union

import assertpy
import numpy as np
import pandas as pd
import pytest
import requests
from botocore.exceptions import BotoCoreError

from feast import Entity, Feature, FeatureService, FeatureView, ValueType
from feast.errors import (
FeatureNameCollisionError,
RequestDataNotFoundInEntityRowsException,
)
from feast.wait import wait_retry_backoff
from tests.integration.feature_repos.repo_configuration import (
Environment,
construct_universal_feature_views,
Expand Down Expand Up @@ -569,7 +571,18 @@ def test_online_store_cleanup(environment, universal_data_sources):
assert np.allclose(expected_values["value"], online_features["value"])

fs.apply(objects=[], objects_to_delete=[simple_driver_fv], partial=False)
fs.apply([simple_driver_fv])

def eventually_apply() -> Tuple[None, bool]:
try:
fs.apply([simple_driver_fv])
except BotoCoreError:
return None, False

return None, True

# Online store backend might have eventual consistency in schema update
# So recreating table that was just deleted might need some retries
wait_retry_backoff(eventually_apply, timeout_secs=60)

online_features = fs.get_online_features(
features=features, entity_rows=entity_rows
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,12 @@ def test_feature_get_online_features_types_match(online_types_test_fixtures):
features = [fv.name + ":value"]
entity = driver(value_type=config.entity_type)
fs.apply([fv, entity])
fs.materialize(environment.start_date, environment.end_date)
fs.materialize(
environment.start_date,
environment.end_date
- timedelta(hours=1) # throwing out last record to make sure
Copy link
Collaborator

Choose a reason for hiding this comment

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

non blocking, but kind of confusing why this ensures we infer from all empty values

# we can successfully infer type even from all empty values
)

driver_id_value = "1" if config.entity_type == ValueType.STRING else 1
online_features = fs.get_online_features(
Expand All @@ -239,9 +244,15 @@ def test_feature_get_online_features_types_match(online_types_test_fixtures):
expected_dtype = feature_list_dtype_to_expected_online_response_value_type[
config.feature_dtype
]

assert len(online_features["value"]) == 1

if config.feature_is_list:
for feature in online_features["value"]:
assert isinstance(feature, list)
assert isinstance(feature, list), "Feature value should be a list"
assert (
config.has_empty_list or len(feature) > 0
), "List of values should not be empty"
for element in feature:
assert isinstance(element, expected_dtype)
else:
Expand Down