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 feat: Track uncertain changes in zulip report #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.

nearcore/runtime/runtime-params-estimator/estimator-warehouse/src/check.rs

Lines 107 to 127 in 78a6456

	fn estimation_changes(
	db: &Db,
	estimation_names: &[String],
	commit_before: &str,
	commit_after: &str,
	tolerance: f64,
	metric: Metric,
	) -> anyhow::Result<Vec<Notice>> {
	let mut warnings = Vec::new();
	for name in estimation_names {
	let b = &EstimationRow::get(db, name, commit_before, metric)?[0];
	let a = &EstimationRow::get(db, name, commit_after, metric)?[0];
	let rel_change = (b.gas - a.gas).abs() / b.gas;
	if rel_change > tolerance {
	warnings.push(Notice::RelativeChange(RelativeChange {
	estimation: name.clone(),
	before: b.gas,
	after: a.gas,
	}))
	}
	}

Second point requires changes in how a ZulipReport is displayed.

nearcore/runtime/runtime-params-estimator/estimator-warehouse/src/zulip.rs

Lines 83 to 108 in 78a6456

	impl std::fmt::Display for ZulipReport {
	fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
	writeln!(f, "## Report ")?;
	writeln!(f, "*Status: {:?}*", self.status)?;
	writeln!(f, "*Current commit: {}*", self.after)?;
	writeln!(f, "*Compared to: {}*", self.before)?;
	writeln!(f, "### Relative gas estimation changes above threshold: {}", self.changes.len())?;
	if self.changes.len() > 0 {
	writeln!(f, "```")?;
	for change in &self.changes {
	let percent_change = 100.0 * (change.after - change.before) / change.before;
	writeln!(
	f,
	"{:<40} {:>16} ➜ {:>16} ({}{:.2}%)",
	change.estimation,
	format_gas(change.before),
	format_gas(change.after),
	if percent_change >= 0.0 { "+" } else { "" },
	percent_change,
	)?;
	}
	writeln!(f, "```")?;
	}
	Ok(())
	}
	}

[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 #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.