[BEAM-9703] Fix user distribution metric test by not validating namespace. #11455
Conversation
|
R: @tvalentyn, @lukecwik |
|
Please run yapf to fix the python formatting. |
|
Did run gradlew lint before commit. Looks like yaqf does things slightly different from gradlew-lint. Now applied "yapf --in-place apache_beam/metrics/metric_test.py". PTAL. |
|
retest this please |
| name='element_dist', | ||
| committed=DistributionMatcher( | ||
| sum_value=10, count_value=4, min_value=1, max_value=4)) | ||
| sum_value=10, count_value=4, |
There was a problem hiding this comment.
why would min value be >= 0 but the other values be exact?
There was a problem hiding this comment.
b/c after repeating it multiple times, streaming side gives consistent outcome on these exact numbers.
After another thought though, given how much uncertainties there are on streaming side, it is really not necessary to put exact number here. And if we do really want to check exact number, there is alternative way to add that separately.
Updated by making all other threes to be 'greater_than(0)'. PTAL.
| sum_value=10, count_value=4, min_value=1, max_value=4)) | ||
| sum_value=hc.greater_than(0), | ||
| count_value=hc.greater_than(0), | ||
| min_value=hc.greater_than_or_equal_to(0), |
There was a problem hiding this comment.
Why is min different then the others?
I would expect that the same reason this needs to be >= 0 would be the same reason as the others.
There was a problem hiding this comment.
Fair enough.
My justification was that as long as there is one single update picked up, then other values should be something non-zero. (e.g. count would be at least one).
Maybe I still underestimated how much uncertainties there is.
Updated. Now they are all >=0.
|
retest this please |
1 similar comment
|
retest this please |
lukecwik
changed the title
|
@lukecwik - does latest revision address your comments? |
Previous validation was too strong.
Alternative way would be to completely disable this test for streaming (and create a dedicated one).
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.