Skip to content
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.

Sign up for GitHub

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 size tracking table #3117

Merged
merged 11 commits into from
Aug 29, 2023
Merged

Improve size tracking table #3117

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4d88fd7
configurable headers + fix change diff sign
jprochazk Aug 28, 2023
52114cb
use better headers when in ci
jprochazk Aug 28, 2023
18827f2
Merge branch 'main' into jan/improve-size-tracking
jprochazk Aug 28, 2023
3a9619f
Use `+` when formatting to preserve sign
jprochazk Aug 28, 2023
66ec193
update variable name
jprochazk Aug 28, 2023
f264a29
fix workflow
jprochazk Aug 28, 2023
69d8de1
try to improve names
jprochazk Aug 29, 2023
1f77420
Merge branch 'main' into jan/improve-size-tracking
jprochazk Aug 29, 2023
d0da78f
more naming
jprochazk Aug 29, 2023
cb697eb
track latest commit using index file
jprochazk Aug 29, 2023
941ec5e
support `N%` values in `--threshold`
jprochazk Aug 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion .github/workflows/reusable_track_size.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,13 @@ jobs:

echo "$data" > "/tmp/data.json"

comparison=$(python3 scripts/ci/sizes.py compare --threshold=5 "/tmp/prev.json" "/tmp/data.json")
comparison=$(
python3 scripts/ci/sizes.py compare \
--threshold=5 \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that sizes.py --help explains the unit of threshold, but that helps very little when reading this file 🤷‍♂️
Not a blocker, just trying to explain my reasoning a bit further.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but I also think that 0.05 is as ambigous as 5 if there's no unit :/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe - but 0.05 is less likely to be of unit Bytes at least :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--threshold-percent would be very clear in any case

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do crimes instead:

comparison=$(
  python3 scripts/ci/sizes.py compare \
    --threshold=5% \
    "prev.json" "data.json"
)

The % is optional

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, now we're talking 😆

--before-header=${{ (inputs.PR_NUMBER && github.event.pull_request.base.ref) || 'Before' }} \
--after-header=${{ github.ref_name }} \
"/tmp/prev.json" "/tmp/data.json"
)
echo "$comparison"

{
Expand Down
34 changes: 26 additions & 8 deletions scripts/ci/sizes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ def render(self, data: list[dict[str, str]]) -> str:
return render_table_dict(data)


def compare(previous_path: str, current_path: str, threshold: float) -> None:
def compare(previous_path: str, current_path: str, threshold: float, before_header: str, after_header: str) -> None:
previous = json.loads(Path(previous_path).read_text())
current = json.loads(Path(current_path).read_text())

Expand All @@ -109,28 +109,26 @@ def compare(previous_path: str, current_path: str, threshold: float) -> None:
entries[name] = {}
entries[name]["previous"] = entry

headers = ["Name", "Previous", "Current", "Change"]
headers = ["Name", before_header, after_header, "Change"]
rows: list[tuple[str, str, str, str]] = []
for name, entry in entries.items():
if "previous" in entry and "current" in entry:
previous = float(entry["previous"]["value"]) * DIVISORS[entry["previous"]["unit"]]
current = float(entry["current"]["value"]) * DIVISORS[entry["current"]["unit"]]

min_change = previous * (threshold / 100)

unit = get_unit(min(previous, current))
div = get_divisor(unit)

change = ((previous / current) * 100) - 100
sign = "+" if change > 0 else ""
change_pct = ((current - previous) / previous) * 100

min_change = previous * (threshold / 100)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The units here confuse me. What unit does threshold have? If it is in percents, why not just compare abs(change_pct) >= threshold ? Again: proper names help a lot.

I prefer to not use percentages except for ui:s (i.e. multiply with 100 during the formatting). It usually ends up with far less 100 in the code (and less bugs). _ratio or _fraction is a pretty good suffix for such numbers. Percentages in arguments have the problem of the unit being invisible. --threshold 0.05 is usually clearer than --threshold 5 (5 kiB?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're about to use the script, you're going to run sizes.py --help first, and the help text for threshold says N%, so --threshold 5 means 5%. As a user, that seems marginally better than 0.05, because the table also outputs percentages. I tell the script I want to see differences of more than 5% and I see all the differences which were more than 5%, no mental math required, however little it may be.

if abs(current - previous) >= min_change:
rows.append(
(
name,
f"{cell(previous, div)} {unit}",
f"{cell(current, div)} {unit}",
f"{sign}{change:.2f}%",
f"{change_pct:+.2f}%",
)
)
elif "current" in entry:
Expand Down Expand Up @@ -186,6 +184,20 @@ def main() -> None:
default=20,
help="Only print row if value is N%% larger or smaller",
)
compare_parser.add_argument(
"--before-header",
type=str,
required=False,
default="Before",
help="Header for before column",
)
compare_parser.add_argument(
"--after-header",
type=str,
required=False,
default="After",
help="Header for after column",
)

measure_parser = cmds_parser.add_parser("measure", help="Measure sizes")
measure_parser.add_argument(
Expand All @@ -200,7 +212,13 @@ def main() -> None:
args = parser.parse_args()

if args.cmd == "compare":
compare(args.before, args.after, args.threshold)
compare(
args.before,
args.after,
args.threshold,
args.before_header,
args.after_header,
)
elif args.cmd == "measure":
measure(args.files, args.format)

Expand Down