prefect icon indicating copy to clipboard operation
prefect copied to clipboard

Clearer presentation of unserializable parameters in the UI

Open Andrew-S-Rosen opened this issue 1 year ago • 4 comments

Goal

When an unserializable parameter is provided to a flow run, a placeholder is stored in the UI. However, the current approach is very opaque, and this PR seeks to improve the user experience in this regard.

Demonstration of the Problem

To demonstrate what I mean, let us consider an example. I have a dictionary provided as a parameter to a flow run that Prefect does not know how to serialize. In the "Parameters" tab of the UI, it shows the following:

{
  "atoms": "<Atoms>"
}

In other words, the value here could not be serialized and was instead transformed to "<Atoms>", which is the name of the class (i.e. Atoms from the library ase). However, if instead of storing the name of the class we instead stored str(value), we would have something potentially much more descriptive (depending on if there is a __repr__ for the unserializable class). Indeed, in this case, we may get something like

{"atoms": "Atoms(symbols='Cu', pbc=True, cell=[[0.0, 1.805, 1.805], [1.805, 0.0, 1.805], [1.805, 1.805, 0.0]])"}`

This is much more informative for the end-user.

This PR

In this PR, I have changed the output in the UI to display str(value) rather than the name of the object. At minimum, it contains the same information as before, but it potentially can provide a much more detailed description.

Checklist

  • [X] This pull request references any related issue by including "closes <link to issue>"
    • If no issue exists and your change is not a small fix, please create an issue first.
  • [X] If this pull request adds new functionality, it includes unit tests that cover the changes
  • [X] If this pull request removes docs files, it includes redirect settings in mint.json.
  • [X] If this pull request adds functions or classes, it includes helpful docstrings.

Andrew-S-Rosen avatar Aug 13 '24 23:08 Andrew-S-Rosen

CodSpeed Performance Report

Merging #14921 will not alter performance

Comparing Andrew-S-Rosen:patch-3 (eed0dfc) with main (ded5dee)

Summary

✅ 3 untouched benchmarks

codspeed-hq[bot] avatar Aug 14 '24 00:08 codspeed-hq[bot]

@cicdw: Brilliant edge case! Yes, you are absolutely right.

I don't think there is anything wrong with truncating the representation to a maximum length (with a ... afterwards). I still think this would provide more insight than just the type. Alternatively, we could default to the type if str(value) is greater than some threshold if you don't like the idea of a truncated str representation. Thoughts?

Andrew-S-Rosen avatar Aug 14 '24 00:08 Andrew-S-Rosen

The pedantic side of me worries that str(value)[:N] will still have client-side edge cases with performance in actually computing the string and I tend to avoid optimizing for display (instead focusing on optimizing for functionality).

That being said, I'm probably overthinking this so I'm OK picking some reasonable value of N and seeing if we run into any issues.

cicdw avatar Aug 14 '24 00:08 cicdw

The pedantic side of me worries that str(value)[:N] will still have client-side edge cases with performance in actually computing the string and I tend to avoid optimizing for display (instead focusing on optimizing for functionality).

This is certainly understandable. In practice, one could imagine a user passing a large dictionary with many key/value pairs, only some of which are not serializable but which collectively make the parameter not serializable as a whole. It might take a few seconds to string-ify it, in particularly extreme cases.

We could do something like sys.getsizeof(). For instance,

import sys
a=["s"]*100000000
sys.getsizeof(a) # outputs 800000056

to decide when to avoid string-ification, but this would be purely subjective. It also won't account for situations where a custom __str__ method might not be implemented efficiently for a given object.

I'm going to leave this open for a short period and think on this for a little bit.

Andrew-S-Rosen avatar Aug 14 '24 01:08 Andrew-S-Rosen

This pull request is stale because it has been open 14 days with no activity. To keep this pull request open remove stale label or comment.

github-actions[bot] avatar Sep 10 '24 16:09 github-actions[bot]

This pull request was closed because it has been stale for 14 days with no activity. If this pull request is important or you have more to add feel free to re-open it.

github-actions[bot] avatar Sep 24 '24 16:09 github-actions[bot]