Support scalar-like analytics values#4659
Conversation
74ee977 to
c3b18c0
Compare
c3b18c0 to
977ddc7
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves analytics logging ergonomics by normalizing “scalar-like” numeric values (e.g., objects with .item() such as NumPy scalars and 0-D arrays) when constructing AnalyticsData, so downstream writers receive built-in Python int/float consistently.
Changes:
- Normalize scalar-like values for
SCALAR/METRICanalytics types duringAnalyticsDatavalidation. - Normalize numeric values inside
SCALARS/METRICSdict payloads. - Add unit tests covering generic scalar-like objects and NumPy scalar / 0-D array inputs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
nvflare/apis/analytix.py |
Adds numeric scalar normalization logic during validation and applies it to scalar and dict-based analytics payloads. |
tests/unit_test/apis/analytix_test.py |
Adds test coverage for scalar-like normalization, including NumPy scalars and 0-D arrays (skipped if NumPy is unavailable). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile SummaryThis PR centralizes normalization of scalar-like analytics values inside
Confidence Score: 4/5Safe to merge; the normalization logic is sound and the existing test suite is largely preserved. The core normalization path is correct and well-tested for the primary use cases. The main gap is that non-normalizable values inside a SCALARS/METRICS dict are silently kept rather than rejected, so a mistyped dict entry would slip past _validate_data_types and only fail later inside a writer. Additionally, the new item() path means numpy booleans are now accepted as scalars where they were previously rejected — a minor but undocumented behavioral expansion. Pay closest attention to the elif data_type in [AnalyticsDataType.METRICS, AnalyticsDataType.SCALARS] normalization loop in nvflare/apis/analytix.py (lines 197-202) and the corresponding test coverage gap for AnalyticsDataType.METRICS in the test file. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[AnalyticsData.__init__] --> B[_validate_data_types]
B --> C{data_type?}
C -->|SCALAR or METRIC| D[_normalize_numeric_scalar]
D --> E{isinstance float or int?}
E -->|Yes| F[return True plus value]
E -->|No| G{has callable item?}
G -->|No| H[return False plus value]
G -->|Yes| I{shape attribute exists?}
I -->|Yes and shape not empty tuple| H
I -->|No or shape is empty tuple| J[call item]
J --> K{result is float or int?}
K -->|Yes| L[return True plus scalar]
K -->|No| H
F --> M[value normalized stored in self.value]
L --> M
H --> N[raise TypeError for SCALAR or METRIC]
C -->|METRICS or PARAMETERS or SCALARS| O{isinstance dict?}
O -->|No| P[raise TypeError]
O -->|Yes| Q{METRICS or SCALARS?}
Q -->|Yes| R[loop normalize each dict entry]
R --> S{entry normalizable?}
S -->|Yes| T[replace with Python float or int]
S -->|No| U[keep original value silently]
T --> V[value = normalized_dict]
U --> V
Q -->|No - PARAMETERS| W[value unchanged]
C -->|TEXT| X{isinstance str?}
X -->|No| Y[raise TypeError]
C -->|TAGS| Z{isinstance dict?}
Z -->|No| AA[raise TypeError]
M --> BB[return value to init]
V --> BB
W --> BB
Reviews (1): Last reviewed commit: "Support scalar-like analytics values" | Re-trigger Greptile |
|
Superseded by replacement PR #4683, which was recreated from latest |
What changed
AnalyticsData.int/floatvalues as before, plus scalar-like objects with.item()such asnumpy.float32and 0-D numpy arrays.SCALARSandMETRICSdictionaries as well.Why
Client tracking writers currently reject values such as
numpy.float32for single scalar metrics because analytics validation only accepts built-in Pythonfloat/int. This can happen with code like:Centralizing the normalization in
AnalyticsDatakeepsSummaryWriter,MLflowWriter,WandBWriter, and lower-level analytics logging paths consistent without adding a numpy dependency to core API code.