-
Notifications
You must be signed in to change notification settings - Fork 998
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 issue with feature views being detected as duplicated when imported #1905
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,7 +11,7 @@ | |
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
import inspect | ||
from typing import Dict, List, MutableMapping, Optional, Union | ||
|
||
import yaml | ||
|
@@ -30,6 +30,7 @@ | |
from feast.protos.feast.core.FeatureTable_pb2 import ( | ||
FeatureTableSpec as FeatureTableSpecProto, | ||
) | ||
from feast.usage import log_exceptions | ||
from feast.value_type import ValueType | ||
|
||
|
||
|
@@ -38,6 +39,7 @@ class FeatureTable: | |
Represents a collection of features and associated metadata. | ||
""" | ||
|
||
@log_exceptions | ||
def __init__( | ||
self, | ||
name: str, | ||
|
@@ -64,6 +66,11 @@ def __init__( | |
self._created_timestamp: Optional[Timestamp] = None | ||
self._last_updated_timestamp: Optional[Timestamp] = None | ||
|
||
stack = inspect.stack() | ||
# Get two levels up from current, to ignore usage.py | ||
previous_stack_frame = stack[2] | ||
self.defined_in = previous_stack_frame.filename | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds like missing one There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh yeah missed this |
||
|
||
def __str__(self): | ||
return str(MessageToJson(self.to_proto())) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -188,6 +188,7 @@ def online_read( | |
|
||
for entity_key in entity_keys: | ||
redis_key_bin = _redis_key(project, entity_key) | ||
print(redis_key_bin) | ||
achals marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be removed ? |
||
hset_keys = [_mmh3(f"{feature_view}:{k}") for k in requested_features] | ||
ts_key = f"_ts:{feature_view}" | ||
hset_keys.append(ts_key) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -107,15 +107,20 @@ def parse_repo(repo_root: Path) -> ParsedRepo: | |
for attr_name in dir(module): | ||
obj = getattr(module, attr_name) | ||
if isinstance(obj, FeatureTable): | ||
res.feature_tables.append(obj) | ||
if obj.defined_in is not None and obj.defined_in == module.__file__: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can module.file be None ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should maybe also raise exceptions in case defined_in is None There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. And no, module.file cannot be None |
||
res.feature_tables.append(obj) | ||
if isinstance(obj, FeatureView): | ||
res.feature_views.append(obj) | ||
if obj.defined_in is not None and obj.defined_in == module.__file__: | ||
res.feature_views.append(obj) | ||
elif isinstance(obj, Entity): | ||
res.entities.append(obj) | ||
if obj.defined_in is not None and obj.defined_in == module.__file__: | ||
res.entities.append(obj) | ||
elif isinstance(obj, FeatureService): | ||
res.feature_services.append(obj) | ||
if obj.defined_in is not None and obj.defined_in == module.__file__: | ||
res.feature_services.append(obj) | ||
elif isinstance(obj, OnDemandFeatureView): | ||
res.on_demand_feature_views.append(obj) | ||
if obj.defined_in is not None and obj.defined_in == module.__file__: | ||
res.on_demand_feature_views.append(obj) | ||
return res | ||
|
||
|
||
|
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.
Should we abstract that in an helper function and also test it in a test file ?
Don't think we need to test each class but at least the helper function
What do you think ?
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.
A helper class probably makes sense since there's some state tracking being done. wdyt @achals ?
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.
yeah even better 👍
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.
Done