-
Notifications
You must be signed in to change notification settings - Fork 297
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
Improve jupyter repr and __repr__
for Flyte models
#2647
Changes from all commits
6cdee7a
26806dd
08837b7
3be15c4
c97a6a7
eaa8736
0faac37
5b055a4
21372e4
f12a555
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 |
---|---|---|
@@ -1,6 +1,9 @@ | ||
import abc | ||
import json | ||
import re | ||
import os | ||
from contextlib import closing | ||
from io import StringIO | ||
from textwrap import shorten | ||
from typing import Dict | ||
|
||
from flyteidl.admin import common_pb2 as _common_pb2 | ||
|
@@ -40,6 +43,29 @@ | |
pass | ||
|
||
|
||
def _repr_idl_yaml_like(idl, indent=0) -> str: | ||
"""Formats an IDL into a YAML-like representation.""" | ||
if not hasattr(idl, "ListFields"): | ||
return str(idl) | ||
|
||
with closing(StringIO()) as out: | ||
for descriptor, field in idl.ListFields(): | ||
try: | ||
inner_fields = field.ListFields() | ||
# if inner_fields is empty, then we do not render the descriptor, | ||
# because it is empty | ||
if inner_fields: | ||
out.write(" " * indent + descriptor.name + ":" + os.linesep) | ||
out.write(_repr_idl_yaml_like(field, indent + 2)) | ||
except AttributeError: | ||
# No ListFields -> Must be a scalar | ||
str_repr = shorten(str(field).strip(), width=80) | ||
if str_repr: | ||
out.write(" " * indent + descriptor.name + ": " + str_repr + os.linesep) | ||
|
||
return out.getvalue() | ||
|
||
|
||
class FlyteIdlEntity(object, metaclass=FlyteType): | ||
def __eq__(self, other): | ||
return isinstance(other, FlyteIdlEntity) and other.to_flyte_idl() == self.to_flyte_idl() | ||
|
@@ -60,9 +86,9 @@ | |
""" | ||
:rtype: Text | ||
""" | ||
literal_str = re.sub(r"\s+", " ", str(self.to_flyte_idl())).strip() | ||
str_repr = _repr_idl_yaml_like(self.to_flyte_idl(), indent=2).rstrip(os.linesep) | ||
type_str = type(self).__name__ | ||
return f"[Flyte Serialized object: Type: <{type_str}> Value: <{literal_str}>]" | ||
return f"Flyte Serialized object ({type_str}):" + os.linesep + str_repr | ||
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 we add a few more tests to cover the different types of literals (collections, maps, etc)? These are useful especially in flyteremote when debugging. 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. I added more tests in 21372e4 |
||
|
||
def verbose_string(self): | ||
""" | ||
|
@@ -73,6 +99,14 @@ | |
def serialize_to_string(self) -> str: | ||
return self.to_flyte_idl().SerializeToString() | ||
|
||
def _repr_html_(self) -> str: | ||
"""HTML repr for object.""" | ||
# `_repr_html_` is used by Jupyter to render objects | ||
type_str = type(self).__name__ | ||
idl = self.to_flyte_idl() | ||
str_repr = _repr_idl_yaml_like(idl).rstrip(os.linesep) | ||
return f"<h4>{type_str}</h4><pre>{str_repr}</pre>" | ||
|
||
@property | ||
def is_empty(self): | ||
return len(self.to_flyte_idl().SerializeToString()) == 0 | ||
|
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.
Why not make this the default repr
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.
I wanted to make it the default, but I saw the current repr strips away the newlines:
flytekit/flytekit/models/common.py
Line 63 in e9f3499
Was there a reason for making it all one line?
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.
i don't think so, not a good one anyways. feel free to overwrite. the reason likely was logging platforms (cloudwatch logs, splunk, grafana, etc) don't render logs with new lines well. but i don't know how much this could matter for python. that's more of a backend issue.