Unified Schema #184
Unified Schema #184
Conversation
This modifies merlin-models to be compatabile with the new 'core' schema class that will be shared with NVTabular
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
| @@ -99,9 +104,9 @@ def read_schema(cls, path: Union[str, Path]) -> Schema: | |||
| os.path.join(str(path), "schema.json") if os.path.isdir(str(path)) else str(path) | |||
| ) | |||
| if _schema_path.endswith(".pb") or _schema_path.endswith(".pbtxt"): | |||
| return Schema().from_proto_text(_schema_path) | |||
| TensorflowMetadata.from_from_proto_text(_schema_path).to_merlin_schema() | |||
There was a problem hiding this comment.
I think this should be from_proto_text_file? Also not sure this works with the way that method is written, since I think the current implementation wants the directory and assumes the file is called schema.pbtxt unless you pass a different value of the filename param. Now wondering if this is covered by tests too...
| query_item_schema = schema.select_by_tag( | ||
| lambda tags: query_id_tag in tags or item_id_tag in tags | ||
| ) | ||
| query_item_schema = schema.select_by_tag(query_id_tag) + schema.select_by_tag(item_id_tag) |
There was a problem hiding this comment.
This seems like a reasonable way to implement the same behavior on the other version of the Schema API. I do wonder if we should think about integrating the lambda functionality into core though.
| if isinstance(item, Tags): | ||
| item = item.value | ||
| else: | ||
| item = str(item) |
There was a problem hiding this comment.
I wonder if this could be a helper on Tags to keep the behavior encapsulated. Something like:
class Tags(Enum):
...
@classmethod
def value(cls, maybe_tag):
if isinstance(maybe_tag, Tags):
return item.value
else:
return str(item)
As pointed out #184 (comment) there was some issues with the SyntheticData.read_schema method. Fix and add a basic unittest that would have caught this
As pointed out #184 (comment) there was some issues with the SyntheticData.read_schema method. Fix and add a basic unittest that would have caught this
This modifies merlin-models to be compatabile with the new 'core' schema class that
will be shared with NVTabular