Fix trailing comma in CSV for verbose_csv and collect_metrics modes

[rmccorm4]

Overview

There are a few places where trailing commas are left depending on config flags:

• Here if should_output_metrics is false
• When writing metrics

Test/verification

Ideally there should be some unit tests to validate the produced CSVs are actually valid CSV files (no trailing delimiters), but verified locally here:

# Gather CSVs for different combinations of flags
./perf_analyzer -m id --shape INPUT0:1,1 --concurrency 1:2 -f report.csv
./perf_analyzer -m id --shape INPUT0:1,1 --concurrency 1:2 -f report_metrics.csv --collect-metrics
./perf_analyzer -m id --shape INPUT0:1,1 --concurrency 1:2 -f report_verbose.csv --verbose-csv
./perf_analyzer -m id --shape INPUT0:1,1 --concurrency 1:2 -f report_verbose_metrics.csv --verbose-csv --collect-metrics

# Verify CSVs set concurrency column correctly instead of getting shifted over
CSVS="report.csv report_metrics.csv report_verbose.csv report_verbose_metrics.csv"
for csv in ${CSVS}; do
  python -c "import pandas as pd; df = pd.read_csv('${csv}'); print(df); assert df['Concurrency'][0] == 1"
done

Side note, nice tool for visual verification via jq:

$ jq -cR 'split(",")' report.csv
["Concurrency","Inferences/Second","Client Send","Network+Server Send/Recv","Server Queue","Server Compute Input","Server Compute Infer","Server Compute Output","Client Recv","p50 latency","p90 latency","p95 latency","p99 latency"]
["1","2281.23","109","284","29","1","12","0","0","413","588","656","806"]
["2","4174.56","120","315","28","1","11","0","0","459","636","701","864"]

[matthewkotila]

@nv-braf are there any consequences to MA from this?

[rmccorm4]

@nv-braf are there any consequences to MA from this?

How is MA consuming these CSVs? I'm surprised it is not failing on parsing the CSVs, or getting incorrect values with columns shifted.

[matthewkotila]

This is technically changing the format of an output a user might expect to be a certain way. I'm not sure how to determine if a user is expecting the trailing comma.

@dyastremsky any input? You were the one we last discussed breaking changes with and how we should be more cautious.

[matthewkotila]

I think it's unlikely someone is expecting a (unnecessary) trailing comma at the end (I don't think it's technically an invalid csv format, but it's implying there's a "null field" at the end, which we don't need to imply), so as long as MA doesn't break from this, I'm cool with getting it merged and making the output cleaner.

[matthewkotila]

I think I misunderstood what this trailing comma is. It looks like it only exists in the data rows, but not in the header row. Yeah, this is a bug (and introduced by me, my bad).

Fix looks good to me--thanks Ryan. Let's just make sure MA can still work. @nv-braf suggestions on running an MA pipeline with these changes?

[rmccorm4]

I think it's unlikely someone is expecting a (unnecessary) trailing comma at the end (I don't think it's technically an invalid csv format, but it's implying there's a "null field" at the end, which we don't need to imply), so as long as MA doesn't break from this, I'm cool with getting it merged and making the output cleaner.

For verbose only. it's true that it is just introducing an additional column that happens to be empty, so technically yes this is introducing a change to remove the empty column:

$ perf_analyzer -m id --shape INPUT0:1,1 -f report_verbose.csv --verbose-csv 

>>> with open("report_verbose.csv") as f:
...   for line in f:
...     print(len(line.split(",")))
... 
17
17

However for verbose + collect-metrics, the number of columns is different in the header row and data row:

$ perf_analyzer -m id --shape INPUT0:1,1 -f report_verbose_metrics.csv --verbose-csv --collect-metrics

>>> with open("report_verbose_metrics.csv") as f:
...   for line in f:
...     print(len(line.split(",")))
... 
20
21

Which I believe should be considered invalid

[nv-braf]

@

MA consumes the <model_name>_results.csv and then calls the csv library to parse the file:

with open(perf_config["latency-report-file"], mode="r") as f: csv_reader = csv.DictReader(f, delimiter=",")

[dyastremsky]

This is technically changing the format of an output a user might expect to be a certain way. I'm not sure how to determine if a user is expecting the trailing comma.

@dyastremsky any input? You were the one we last discussed breaking changes with and how we should be more cautious.

Thanks for looping me in. This seems like a solid change. Seems like we're fixing previously broken behavior, which would have positive impact for customers. I assume the risk of breaking anything for customers with this change to be fairly close to zero.

I do agree though that additional testing would be helpful. That would require us to explicitly decide the expected format and validate it automatically after future PRs.

[nv-braf]

I think I misunderstood what this trailing comma is. It looks like it only exists in the data rows, but not in the header row. Yeah, this is a bug (and introduced by me, my bad).

Fix looks good to me--thanks Ryan. Let's just make sure MA can still work. @nv-braf suggestions on running an MA pipeline with these changes?

You should be able to run the MA CI with a custom version of PA. I would ask Mischa to help you with this.

[matthewkotila]

@nv-braf ran locally main branch of MA with a csv output from PA that doesn't have the trailing comma and it worked fine. Good enough verification for me for now. Approving.