Continuous estimation: Display uncertain warning in Zulip

[jakmeier]

Right now, results marked as uncertain are displayed just like any other results in Murphy's Zulip chat.

Proposed way to show it:

1. ~When a previously certain results turns uncertain, also show it once in the Zulip chat.~ (Done in #7359
2. When a diff shows up, also display if the result is certain or not.

(Different solution are also acceptable.)

The first point could be done by writing a new method estimator_uncertain_results similar to estimation_changes. https://github.com/near/nearcore/blob/78a645681b3b129b9470f23ee185338680cc19d4/runtime/runtime-params-estimator/estimator-warehouse/src/check.rs#L107-L127

Second point requires changes in how a ZulipReport is displayed. https://github.com/near/nearcore/blob/78a645681b3b129b9470f23ee185338680cc19d4/runtime/runtime-params-estimator/estimator-warehouse/src/zulip.rs#L83-L108

[ghost]

Hello @jakmeier I would like to work on #1. Let me know if its okay. Also, is the below report format works?

Format:

                writeln!(
                    f,
                    "{:<40} {:>40} ➜ {:>40}",
                    change.estimation,
                    change.before,
                    change.after,
                )?;

Sample report:

Report

Status: Warn Current commit: 0004a Compared to: 0003a

Relative gas estimation changes above threshold: 1

LogBase                                         4.00 Ggas ➜        5.00 Ggas (+25.00%)

Gas estimator uncertain changes: 1

LogBase                                                                      None ➜                                something

[jakmeier]

Hi @DevSabb,

Yes you are very much welcome to work on this! Thank you for your interest! :)

I like the suggested format. Your example isn't 100% clear on this, how would you display an uncertain reason field with the value Some("HIGH-VARIANCE")? I am thinking something like None ➜ HIGH-VARIANCE maybe?

Also, sorry for the delayed response, I was mostly offline for 2 weeks and just catching up on all notifications and messages. I should be more responsive from now on, so feel free to ping me anytime something is unclear.

[ghost]

Thanks for your response. I have some followup.

What's the expected max length of the String uncertain reason? I am using width of 40 for name, old and new uncertain reason. The 40 comes of current report which uses the width of name as 40. I am also not sure how the report will be formatted if the string length exceeds 40.

Right now, I am aligning name to left, old and new uncertain reason to the right. But based on your response, I think I should align name and new uncertain reason to the left and old uncertain reason to the right. Let me know if you think otherwise.

[jakmeier]

Currently known values are:

BLOCK-MEASUREMENT-OVERHEAD
HIGH-VARIANCE
NEG-LEAST-SQUARES
NEGATIVE-COST
SUBTRACTION-UNDERFLOW

The longest is 26 characters. It is possible that we will have longer strings at some point. If it happens, we can simply update the formatting code. But I think even 26 characters look pretty ugly to me, so I will object any name that is more than, say 32 characters anyway. ;)

I worry a bit about the total length of a line. It should be all visible without scrolling on a normal desktop or laptop screen. So maybe let's use "{:<40} {:>32} ➜ {:<32}" instead of 40 three times and see how that looks in Zulip. But changing that number is trivial, so I would not get hung up by this too much.

With regards to left or right alignment, I strongly feel that numbers should be right-aligned, like in the relative changes. For strings, I have no strong opinion. It is down to personal taste and I think this is in the freedom of the implementer to decide.

[matklad]

Closed by https://github.com/near/nearcore/pull/7359 ?

[ghost]

@matklad I don't think so. There are two features requested in the original post. I completed #1. #2 is still up for grabs.

[jakmeier]

Yes, exactly. So far, we only see when the uncertain reason changes. If the uncertain flag stays the same but it has relative changes > 10%, it should be marked clearly in the output that the results are uncertain in the first place.

[jakmeier]

I believe instead of showing uncertain results only once, we should probably always show a list of uncertain results. When I initially wrote this issue, this would have been quite verbose. But now, there should only be 1 or 2 uncertain results. And if we don't display it, we will forget that those results actually are uncertain.