-
Notifications
You must be signed in to change notification settings - Fork 260
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
Improve jupyter repr and __repr__
for Flyte models
#2647
Conversation
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
@@ -40,6 +44,29 @@ def from_flyte_idl(cls, idl_object): | |||
pass | |||
|
|||
|
|||
def _repr_idl_yaml_like(idl, indent=0) -> str: |
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
literal_str = re.sub(r"\s+", " ", str(self.to_flyte_idl())).strip() |
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.
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
…ve_repr_jupyter Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
__repr__
for Flyte models
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
I added more tests in 21372e4
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
…ve_repr_jupyter Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2647 +/- ##
==========================================
+ Coverage 78.69% 80.51% +1.81%
==========================================
Files 187 286 +99
Lines 19257 23763 +4506
Branches 4029 3814 -215
==========================================
+ Hits 15155 19132 +3977
- Misses 3404 3928 +524
- Partials 698 703 +5 ☔ View full report in Codecov by Sentry. |
Why are the changes needed?
Improves jupyter repr for
flytekit.models
.What changes were proposed in this pull request?
This PR adds a
_repr_html_
that generically renders anyflytekit.model
. Internally it uses_repr_idl_yaml_like
that renders any flyteidl as a yaml like repr.How was this patch tested?
I added a simple unit test.
Screenshots
This PR
Current