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

Frontend metrics configuration handling #2190

Merged

Conversation

namannandan
Copy link
Collaborator

Description

Implementation of frontend metrics configuration parsing and validation.
Design: #1492

Fixes #2134

Type of change

  • New feature (non-breaking change which adds functionality)

Feature/Issue validation/testing

  • Unit tests included with PR run as part of sanity test

Checklist:

  • Did you have fun?
  • Have you added tests that prove your fix is effective or that this feature works?
  • Has code been commented, particularly in hard-to-understand areas?
  • Have you made corresponding changes to the documentation?

@namannandan namannandan changed the title Frontend metric configuration handling Frontend metrics configuration handling Mar 22, 2023
@namannandan namannandan marked this pull request as ready for review March 22, 2023 18:19
@codecov
Copy link

codecov bot commented Mar 22, 2023

Codecov Report

Merging #2190 (d9ace25) into master (dd8e792) will decrease coverage by 0.09%.
The diff coverage is n/a.

❗ Current head d9ace25 differs from pull request most recent head 9970acc. Consider uploading reports for the commit 9970acc to get more accurate results

@@            Coverage Diff             @@
##           master    #2190      +/-   ##
==========================================
- Coverage   71.47%   71.38%   -0.09%     
==========================================
  Files          73       73              
  Lines        3341     3341              
  Branches       57       57              
==========================================
- Hits         2388     2385       -3     
- Misses        950      953       +3     
  Partials        3        3              

see 1 file with indirect coverage changes

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

Copy link
Collaborator

@lxning lxning left a comment

Choose a reason for hiding this comment

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

TsMetrics and ModelMetrics are exactly same. it is not necessary to have them.

Only need:

  • MetricConfiguraiton.java
  • MetricSpec.java

@namannandan
Copy link
Collaborator Author

Addressed review comments:

  • Consolidated TsMetrics and ModelMetrics into a single class MetricTypes to avoid code duplication.
  • Removed YAMLMetricsConfigurationHandler and moved configuration loader function into MetricConfiguration class.
  • Added support for metrics mode configuration environment variable.

@namannandan namannandan force-pushed the naman-metrics-frontend-config-handling branch from 88d4172 to 6bb17d6 Compare April 1, 2023 00:48
@namannandan namannandan requested a review from lxning April 1, 2023 00:51
@namannandan namannandan requested a review from lxning April 4, 2023 09:21
@namannandan namannandan requested a review from lxning April 11, 2023 00:33
@namannandan namannandan force-pushed the naman-metrics-frontend-config-handling branch from 31c1d6d to 0576bd4 Compare April 12, 2023 00:03
@namannandan namannandan requested a review from lxning April 12, 2023 20:13
@namannandan namannandan requested a review from lxning April 20, 2023 02:56
@namannandan namannandan merged commit b03ac7d into pytorch:master Apr 20, 2023
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.

Frontend metrics yaml parser
3 participants