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.

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 metric unit assignment #2693

Merged
merged 3 commits into from
Oct 11, 2023

Conversation

namannandan
Copy link
Collaborator

@namannandan namannandan commented Oct 10, 2023

Description

In metric_abstract.py, the unit attribute is not assigned if it is not one of the units defined in unit.py

if unit in list(MetricUnit.units.keys()):
self.unit = MetricUnit.units[unit]

This can lead to the following error when attempting to emit the metric:

stdout MODEL_LOG - AttributeError: 'CachingMetric' object has no attribute 'unit'

Fixes #2637

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Feature/Issue validation/testing

  • Unit test included in this PR that runs as part of CI

  • Manual test
    Testing the following call to emit metric in handler code across different versions

self.context.metrics.add_metric('batchSize', len(data), 'request', dimensions=[Dimension(name="ModelName", value=self.context.model_name), Dimension(name="Level", value="Model")])

v0.6.0

2023-10-09T16:29:59,894 [INFO ] W-9009-mnist_1.0-stdout MODEL_METRICS - batchSize.request:1|#ModelName:mnist,Level:Model|#hostname:88665a372f4b.ant.amazon.com,requestID:136830d9-a9b0-4450-940e-2b1b6b01e0e5,timestamp:1696894199

v0.8.2

2023-10-09T16:32:42,152 [INFO ] W-9006-mnist_1.0-stdout MODEL_LOG - AttributeError: 'CachingMetric' object has no attribute 'unit'

With fix in this PR

2023-10-09T16:41:42,765 [INFO ] W-9006-mnist_1.0-stdout MODEL_METRICS - batchSize.request:1.0|#ModelName:mnist,Level:Model|#hostname:88665a372f4b.ant.amazon.com,requestID:3d5f3d3d-e927-415a-ba1f-c95baef60987,timestamp:1696894902

@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Merging #2693 (37dfaa2) into master (4d6dbe6) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head 37dfaa2 differs from pull request most recent head 5804690. Consider uploading reports for the commit 5804690 to get more accurate results

@@            Coverage Diff             @@
##           master    #2693      +/-   ##
==========================================
+ Coverage   72.39%   72.44%   +0.04%     
==========================================
  Files          85       85              
  Lines        3956     3963       +7     
  Branches       58       58              
==========================================
+ Hits         2864     2871       +7     
  Misses       1088     1088              
  Partials        4        4              
Files Coverage Δ
ts/metrics/metric_abstract.py 93.33% <100.00%> (+0.47%) ⬆️
...sts/metrics_yaml_testing/metric_cache_unit_test.py 99.68% <100.00%> (+<0.01%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@msaroufim msaroufim self-requested a review October 10, 2023 20:55
@namannandan namannandan added this pull request to the merge queue Oct 10, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 10, 2023
@namannandan namannandan added this pull request to the merge queue Oct 10, 2023
Merged via the queue into pytorch:master with commit 3906e0b Oct 11, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metric non-default unit is not set in Metric object in versions 0.6.1 onwards
2 participants