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

Fix typing visibility for ExporterConfig #232

Merged
merged 1 commit into from
Dec 31, 2024
Merged

Fix typing visibility for ExporterConfig #232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
67 changes: 12 additions & 55 deletions genai-perf/genai_perf/export_data/exporter_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,62 +25,19 @@
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.


import argparse as args
from dataclasses import dataclass
from pathlib import Path
from typing import Any, Dict, Optional

from genai_perf.metrics import Metrics


@dataclass
class ExporterConfig:
def __init__(self):
self._stats = None
self._telemetry_stats = None
self._metrics = None
self._args = None
self._extra_inputs = None
self._artifact_dir = None

@property
def stats(self):
return self._stats

@stats.setter
def stats(self, stats_value):
self._stats = stats_value

@property
def telemetry_stats(self):
return self._telemetry_stats

@telemetry_stats.setter
def telemetry_stats(self, stats_value):
self._telemetry_stats = stats_value

@property
def metrics(self):
return self._metrics

@metrics.setter
def metrics(self, metrics: Metrics):
self._metrics = metrics

@property
def args(self):
return self._args

@args.setter
def args(self, args_value):
self._args = args_value

@property
def extra_inputs(self):
return self._extra_inputs

@extra_inputs.setter
def extra_inputs(self, extra_inputs_value):
self._extra_inputs = extra_inputs_value

@property
def artifact_dir(self):
return self._artifact_dir

@artifact_dir.setter
def artifact_dir(self, artifact_dir_value):
self._artifact_dir = artifact_dir_value
stats: Dict[Any, Any]
metrics: Metrics
args: args.Namespace
extra_inputs: Dict[str, Any]
artifact_dir: Path
telemetry_stats: Optional[Dict[str, Any]] = None
21 changes: 13 additions & 8 deletions genai-perf/genai_perf/export_data/output_reporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@

from genai_perf.export_data.data_exporter_factory import DataExporterFactory
from genai_perf.export_data.exporter_config import ExporterConfig
from genai_perf.metrics import Statistics, TelemetryStatistics
from genai_perf.metrics import Metrics, Statistics, TelemetryStatistics
from genai_perf.subcommand.common import get_extra_inputs_as_dict


Expand Down Expand Up @@ -60,13 +60,18 @@ def report_output(self) -> None:
exporter.export()

def _create_exporter_config(self) -> ExporterConfig:
config = ExporterConfig()
config.stats = self.stats.stats_dict
config.telemetry_stats = (
assert isinstance(self.stats.metrics, Metrics)
extra_inputs = get_extra_inputs_as_dict(self.args)
telemetry_stats = (
self.telemetry_stats.stats_dict if self.telemetry_stats else None
)
config.metrics = self.stats.metrics
config.args = self.args
config.artifact_dir = self.args.artifact_dir
config.extra_inputs = get_extra_inputs_as_dict(self.args)
config = ExporterConfig(
self.stats.stats_dict,
self.stats.metrics,
self.args,
extra_inputs,
self.args.artifact_dir,
telemetry_stats,
)

return config
9 changes: 8 additions & 1 deletion genai-perf/genai_perf/metrics/telemetry_statistics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,18 @@ def __init__(self, metrics: TelemetryMetrics):
for attr, data in self._metrics.data.items():
if self._should_skip(data):
continue

gpu_index = None
gpu_data = None

for gpu_index, gpu_data in data.items():
self._stats_dict[attr][gpu_index]["avg"] = (
self._statistics._calculate_mean(gpu_data)
)
if not self._is_constant_metric(attr):
if gpu_data is None or gpu_index is None:
matthewkotila marked this conversation as resolved.
Show resolved Hide resolved
continue

percentile_results = self._statistics._calculate_percentiles(gpu_data)
for percentile_label, percentile_value in percentile_results.items():
self._stats_dict[attr][gpu_index][
Expand Down Expand Up @@ -121,5 +128,5 @@ def _is_constant_metric(self, attr: str) -> bool:
return attr in ["gpu_power_limit", "total_gpu_memory"]

@property
def stats_dict(self) -> Dict:
def stats_dict(self) -> Dict[str, Any]:
return self._stats_dict
4 changes: 2 additions & 2 deletions genai-perf/genai_perf/subcommand/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
import os
import subprocess # nosec
from argparse import Namespace
from typing import List, Optional
from typing import Any, Dict, List, Optional

import genai_perf.logging as logging
from genai_perf.config.generate.perf_analyzer_config import PerfAnalyzerConfig
Expand Down Expand Up @@ -81,7 +81,7 @@ def calculate_metrics(args: Namespace, tokenizer: Tokenizer) -> ProfileDataParse
)


def get_extra_inputs_as_dict(args: Namespace) -> dict:
def get_extra_inputs_as_dict(args: Namespace) -> Dict[str, Any]:
request_inputs = {}
if args.extra_inputs:
for input_str in args.extra_inputs:
Expand Down
70 changes: 36 additions & 34 deletions genai-perf/tests/test_exporters/test_console_exporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
TelemetryMetrics,
TelemetryStatistics,
)
from tests.test_utils import create_default_exporter_config


class TestConsoleExporter:
Expand Down Expand Up @@ -69,11 +70,10 @@ def test_streaming_llm_output(self, monkeypatch, capsys) -> None:
input_sequence_lengths=[5, 6, 7],
)
stats = Statistics(metrics=metrics)

config = ExporterConfig()
config.stats = stats.stats_dict
config.metrics = stats.metrics
config.args = args
assert isinstance(stats.metrics, Metrics)
config = create_default_exporter_config(
stats=stats.stats_dict, metrics=stats.metrics, args=args
)

exporter = ConsoleExporter(config)
exporter.export()
Expand Down Expand Up @@ -124,10 +124,10 @@ def test_nonstreaming_llm_output(self, monkeypatch, capsys) -> None:
)
stats = Statistics(metrics=metrics)

config = ExporterConfig()
config.stats = stats.stats_dict
config.metrics = stats.metrics
config.args = args
assert isinstance(stats.metrics, Metrics)
Copy link
Contributor Author

@dyastremsky dyastremsky Dec 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

These asserts are necessary because the current architecture means that stats.metrics can be Metrics or TelemetryMetrics. It is beyond the scope of this PR to separate the two, but we do need asserts like this to check the correct type is used and to not result in type checking failures. It was previously opaque that we were using two types in the exporter config.

We could always tackle it in a future cleanup effort if it causes issues, but making the typing transparent should help.

config = create_default_exporter_config(
stats=stats.stats_dict, metrics=stats.metrics, args=args
)

exporter = ConsoleExporter(config)
exporter.export()
Expand Down Expand Up @@ -170,10 +170,10 @@ def test_embedding_output(self, monkeypatch, capsys) -> None:
)
stats = Statistics(metrics=metrics)

config = ExporterConfig()
config.stats = stats.stats_dict
config.metrics = stats.metrics
config.args = args
assert isinstance(stats.metrics, Metrics)
config = create_default_exporter_config(
stats=stats.stats_dict, metrics=stats.metrics, args=args
)

exporter = ConsoleExporter(config)
exporter.export()
Expand Down Expand Up @@ -222,10 +222,10 @@ def test_valid_goodput(self, monkeypatch, capsys) -> None:
)
stats = Statistics(metrics=metrics)

config = ExporterConfig()
config.stats = stats.stats_dict
config.metrics = stats.metrics
config.args = args
assert isinstance(stats.metrics, Metrics)
config = create_default_exporter_config(
stats=stats.stats_dict, metrics=stats.metrics, args=args
)

exporter = ConsoleExporter(config)
exporter.export()
Expand Down Expand Up @@ -281,10 +281,10 @@ def test_invalid_goodput_output(self, monkeypatch, capsys) -> None:

stats = Statistics(metrics=metrics)

config = ExporterConfig()
config.stats = stats.stats_dict
config.metrics = stats.metrics
config.args = args
assert isinstance(stats.metrics, Metrics)
config = create_default_exporter_config(
stats=stats.stats_dict, metrics=stats.metrics, args=args
)

exporter = ConsoleExporter(config)
exporter.export()
Expand Down Expand Up @@ -345,10 +345,10 @@ def test_console_title(

stats = Statistics(metrics=metrics)

config = ExporterConfig()
config.stats = stats.stats_dict
config.metrics = stats.metrics
config.args = args
assert isinstance(stats.metrics, Metrics)
config = create_default_exporter_config(
stats=stats.stats_dict, metrics=stats.metrics, args=args
)

exporter = ConsoleExporter(config)
exporter.export()
Expand Down Expand Up @@ -395,11 +395,13 @@ def test_valid_telemetry_verbose(self, monkeypatch, capsys) -> None:
stats = Statistics(metrics=metrics)
telemetry_stats = TelemetryStatistics(telemetry_metrics)

config = ExporterConfig()
config.stats = stats.stats_dict
config.telemetry_stats = telemetry_stats.stats_dict
config.metrics = stats.metrics
config.args = args
assert isinstance(stats.metrics, Metrics)
config = create_default_exporter_config(
stats=stats.stats_dict,
metrics=stats.metrics,
args=args,
telemetry_stats=telemetry_stats.stats_dict,
)

exporter = ConsoleExporter(config)
exporter.export()
Expand Down Expand Up @@ -501,10 +503,10 @@ def test_missing_data(self, monkeypatch, capsys) -> None:
)
stats = Statistics(metrics=metrics)

config = ExporterConfig()
config.stats = stats.stats_dict
config.metrics = stats.metrics
config.args = args
assert isinstance(stats.metrics, Metrics)
config = create_default_exporter_config(
stats=stats.stats_dict, args=args, metrics=stats.metrics
)

# Missing data
del config.stats["request_latency"]["avg"]
Expand Down
Loading
Loading