Skip to content

Commit d2e49d0

Browse files
Address review: preserve original config objects; fix SPDX year
Per review (acollins3, Alok-Joshi): - Group unhashable config keys (dict/list) on a temporary stringified key and recover the originals via a "first" aggregation, so the output keeps the real objects (e.g. dims stays a dict, usable without re-parsing) instead of their repr. The helper key is dropped before returning, and normalization merges on it so dict configs work there too. - Bump the new test file's SPDX year to 2026. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent c7fd791 commit d2e49d0

2 files changed

Lines changed: 54 additions & 28 deletions

File tree

‎nsight/transformation.py‎

Lines changed: 37 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -49,23 +49,6 @@ def aggregate_data(
4949
# Note: When num_args=0, we need an empty list (not all columns via [-0:])
5050
func_fields = df.columns[-num_args:].tolist() if num_args > 0 else []
5151

52-
# Stringify columns that can't be groupby keys. Keys must be hashable
53-
# (pandas factorizes them) and sortable (groupby sorts them); sorted() alone
54-
# misses unhashable values in <=1-row columns since no comparison runs, so
55-
# probe hashability with set() too.
56-
def convert_non_sortable_columns(dframe: pd.DataFrame) -> pd.DataFrame:
57-
for col in dframe.columns:
58-
non_null = dframe[col].dropna()
59-
try:
60-
set(non_null)
61-
sorted(non_null)
62-
except (TypeError, ValueError):
63-
dframe[col] = dframe[col].astype(str)
64-
return dframe
65-
66-
# Convert non-sortable columns before grouping
67-
df = convert_non_sortable_columns(df)
68-
6952
# Preserve original order by adding an index column
7053
df = df.reset_index(drop=True)
7154
df["_original_order"] = df.index
@@ -112,12 +95,41 @@ def convert_non_sortable_columns(dframe: pd.DataFrame) -> pd.DataFrame:
11295
)(col),
11396
)
11497

98+
# Group keys must be hashable (pandas factorizes them) and sortable
99+
# (groupby sorts them). Keys that are neither — e.g. dict- or list-valued
100+
# config parameters — are grouped on a temporary stringified key, and the
101+
# original objects are recovered with a "first" aggregation so the output
102+
# preserves them (a dict stays a dict instead of becoming its repr).
103+
def _is_groupable(series: pd.Series) -> bool:
104+
non_null = series.dropna()
105+
try:
106+
set(non_null)
107+
sorted(non_null)
108+
except (TypeError, ValueError):
109+
return False
110+
return True
111+
112+
sortkey_cols: list[str] = []
113+
group_keys: list[str] = []
114+
func_field_keys: list[str] = []
115+
for col in groupby_columns + func_fields:
116+
if _is_groupable(df[col]):
117+
key = col
118+
else:
119+
key = f"{col}__sortkey__"
120+
df[key] = df[col].astype(str)
121+
sortkey_cols.append(key)
122+
named_aggs[col] = (col, "first") # preserve the original objects
123+
group_keys.append(key)
124+
if col in func_fields:
125+
func_field_keys.append(key)
126+
115127
# Ensure Value column is numeric (explode can leave it as object dtype
116128
# even though the underlying values are always numeric from NCU metrics)
117129
df["Value"] = pd.to_numeric(df["Value"])
118130

119131
# Apply aggregation with named aggregation
120-
groupby_df = df.groupby(groupby_columns + func_fields, dropna=False)
132+
groupby_df = df.groupby(group_keys, dropna=False)
121133
agg_df = groupby_df.agg(**named_aggs).reset_index()
122134

123135
# Compute 95% confidence intervals
@@ -152,8 +164,9 @@ def convert_non_sortable_columns(dframe: pd.DataFrame) -> pd.DataFrame:
152164
normalize_against in agg_df["Annotation"].values
153165
), f"Annotation '{normalize_against}' not found in data."
154166

155-
# Columns of normalization dataframe to merge on
156-
merge_on = [*func_fields, "Metric"]
167+
# Columns of normalization dataframe to merge on (use the stringified
168+
# keys for any unhashable config columns so the merge can hash them)
169+
merge_on = [*func_field_keys, "Metric"]
157170

158171
# Create a DataFrame to hold the normalization values
159172
normalization_df = agg_df[agg_df["Annotation"] == normalize_against][
@@ -181,4 +194,8 @@ def convert_non_sortable_columns(dframe: pd.DataFrame) -> pd.DataFrame:
181194
)
182195
)
183196

197+
# Drop the temporary stringified group keys; the original columns remain.
198+
if sortkey_cols:
199+
agg_df = agg_df.drop(columns=sortkey_cols)
200+
184201
return agg_df

‎tests/test_transformation.py‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
# SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
1+
# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
22
# SPDX-License-Identifier: Apache-2.0
33

44
"""Unit tests for nsight.transformation.aggregate_data (no GPU required)."""
@@ -30,13 +30,17 @@ def _raw_df(config_values: list[Any], *, param_name: str = "config") -> pd.DataF
3030

3131
def test_dict_config_single_row_does_not_crash() -> None:
3232
# Regression: a single dict-config row used to crash groupby().agg() with
33-
# "unhashable type: 'dict'" because the dict was never stringified.
33+
# "unhashable type: 'dict'". The output must keep the dict as a real dict.
3434
dims = {"M": 512, "K": 512, "N": 512}
3535
result = transformation.aggregate_data(_raw_df([dims]), _one_arg, None, False)
3636
assert len(result) == 1
37-
assert result["config"].iloc[0] == str(dims)
37+
assert result["config"].iloc[0] == dims
38+
assert isinstance(result["config"].iloc[0], dict)
39+
assert result["config"].iloc[0]["M"] == 512 # usable as a dict, no parsing
3840
assert result["NumRuns"].iloc[0] == 1
3941
assert result["AvgValue"].iloc[0] == 1.0
42+
# The temporary sort key must not leak into the output.
43+
assert not any("sortkey" in c for c in result.columns)
4044

4145

4246
def test_dict_config_independent_of_run_count() -> None:
@@ -48,22 +52,27 @@ def test_dict_config_independent_of_run_count() -> None:
4852
)
4953
assert len(result) == 1
5054
assert result["NumRuns"].iloc[0] == runs
55+
assert result["config"].iloc[0] == dims
5156

5257

5358
def test_distinct_dict_configs_stay_distinct() -> None:
54-
# Stringification is per-row, so different dicts remain different groups.
55-
d1, d2 = {"M": 512}, {"M": 1024}
59+
# Different dicts form different groups, each preserved as a real dict.
60+
d1 = {"M": 512, "K": 512, "N": 512}
61+
d2 = {"M": 1024, "K": 1024, "N": 1024}
5662
result = transformation.aggregate_data(_raw_df([d1, d2]), _one_arg, None, False)
5763
assert len(result) == 2
58-
assert set(result["config"]) == {str(d1), str(d2)}
64+
configs = list(result["config"])
65+
assert all(isinstance(c, dict) for c in configs)
66+
assert d1 in configs and d2 in configs
5967

6068

6169
def test_list_config_single_row_does_not_crash() -> None:
62-
# Lists are unhashable too.
70+
# Lists are unhashable too, and must also be preserved as lists.
6371
shape = [512, 512]
6472
result = transformation.aggregate_data(_raw_df([shape]), _one_arg, None, False)
6573
assert len(result) == 1
66-
assert result["config"].iloc[0] == str(shape)
74+
assert result["config"].iloc[0] == shape
75+
assert isinstance(result["config"].iloc[0], list)
6776

6877

6978
def test_numeric_config_keeps_native_dtype() -> None:

0 commit comments

Comments
 (0)