con / duct

A helper to run a command, capture stdout/stderr and details about running
https://pypi.org/project/con-duct/
MIT License
1 stars 2 forks source link

Do format % for CPU in summary sensibly/uniformly #171

Open yarikoptic opened 2 weeks ago

yarikoptic commented 2 weeks ago

I thought we had an issue for that, so feel free to close with a reference.

In a fresh https://github.com/con/duct/issues/170 I saw

    CPU Peak Usage: 12.2%
    Average CPU Usage: 6.457543859649121%

which should really be

    CPU Peak Usage: 12.2%
    Average CPU Usage: 6.4%

or alike -- there is really no value in those gory details for this one.

asmacdo commented 2 weeks ago

@yarikoptic we have had some similar ones in the past. It would be easy to round all of the values in the "human readable" serializer. Currently, we do the rounding in the format string, which I had hoped would allow users to override the rounding. But the problem is that those strings can't handle non-float type errors.

EXECUTION_SUMMARY_FORMAT_DEFAULT = (
    "Wall Clock Time: {wall_clock_time:.3f} sec\n"
    "Memory Average Usage (RSS): {average_rss:.3f} bytes\n"
)

But before I fix this, I am advocating that we instead drop the --summary-format feature instead, which I posed (in the exit-code issue)

IMO this feature isn't super useful since this information is written in machine readable format in the info.json, so if the user wants to do anything with it, it would be better to use the file directly rather than parsing stderr logs.

yarikoptic commented 2 weeks ago

before we drop anything -- I expect that all numbers are stored numerically in the record so you should be able to add similar :.1f or alike. Isn't that the case?

asmacdo commented 2 weeks ago

No, in the case that we don't get a sample some of the values are None (which we translate to "unknown")

yarikoptic commented 1 week ago

then could be {"unknown" if blah is None else "%.1f" % blah} or alike. you could probably even add a helper, like format_maybe_undefined and use {format_maybe_undefined(".1!f", blah)} or alike.

asmacdo commented 1 week ago

user-provided format strings are strings, not f-strings. See also https://github.com/con/duct/issues/172

yarikoptic commented 1 week ago

here is a cute solution chatgpt inspired

class MaybeNone:
    def __init__(self, value):
        self.value = value

    def __format__(self, format_spec):
        if self.value is None:
            return "unknown"
        else:
            return format(self.value, format_spec)

# Usage
unknown = MaybeNone(None)
pi = MaybeNone(3.14159)

print("{:.2f}".format(unknown))  # Output: unknown
print("{:.2f}".format(pi))  # Output: 3.14

so you can wrap all values with the construct and have "unknown" to be listed for all Nones . Could make it even more sophisticated to handle empty lists as "no records", etc.

asmacdo commented 1 week ago

Could this pattern allow the user to specify units though? As a user myself, I'd really like to be able to specify gigabytes so I don't have to count digits. But it's hard to imagine that feature could play nice with user-provided format strings.

yarikoptic commented 1 week ago

We could avoid somehow figuring out "units" but rather provide for each field, a humanized version (string) as well, e.g. to have wall_clock_time to be e.g. 3600 and wall_clock_time_human to be "1 hr". Then user can decide which one to use. Although yet to check on format options, may be would make sense to add some.