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 float64 comparison test failure on archs using FMA #1133

Merged
merged 4 commits into from Nov 7, 2022

Conversation

sbunce
Copy link
Contributor

@sbunce sbunce commented Sep 13, 2022

Background

Architectures using FMA optimization yield slightly different results so we cannot assume floating point values will be precisely the same across different architectures.
#899 (comment)

Change

The solution in this change is to check "abs(a-b) < tolerance" instead of comparing the exact values. This will give us confidence that the histogram buckets are near identical.

Testing

I have not tested this change on one of the previously failing architectures. Help from someone with access to one of these architectures would be appreciated.

@sbunce sbunce force-pushed the sbunce/fix-fp-test branch 3 times, most recently from e8353d9 to 912193e Compare September 13, 2022 16:59
Architectures using FMA optimization yield slightly different results so
we cannot assume floating point values will be precisely the same across
different architectures.

The solution in this change is to check "abs(a-b) < tolerance" instead
of comparing the exact values. This will give us confidence that the
histogram buckets are near identical.

Signed-off-by: Seth Bunce <seth.bunce@getcruise.com>
@sbunce sbunce marked this pull request as ready for review September 13, 2022 17:11
prometheus/histogram_test.go Outdated Show resolved Hide resolved
Co-authored-by: Daniel Swarbrick <daniel.swarbrick@gmail.com>
Signed-off-by: Seth Bunce <seth.bunce@getcruise.com>
@beorn7
Copy link
Member

beorn7 commented Sep 20, 2022

There are better ways of a tolerant comparison. We already have https://github.com/prometheus/prometheus/blob/main/promql/test.go#L637 in the Prometheus code base. Perhaps that's easy enough to just copy over here.

@beorn7
Copy link
Member

beorn7 commented Sep 20, 2022

Or depend on https://github.com/beorn7/floats :o)

@dswarbrick
Copy link
Contributor

Or depend on https://github.com/beorn7/floats :o)

I would prefer not to have to package yet another tiny dependency to make this build on Debian (and derivatives), so I would advocate just copying that function.

Or use something like https://pkg.go.dev/github.com/stretchr/testify/assert?utm_source=godoc#InDeltaSlice

@beorn7
Copy link
Member

beorn7 commented Sep 27, 2022

Yeah, the dependency was meant as a joke. I think the technique implemented in https://github.com/prometheus/prometheus/blob/main/promql/test.go#L637 (or in a slightly more general form in https://github.com/beorn7/floats ) works better that the approach in testify. And yes, I would propose to just copy that piece of code, maybe to the already existing prometheus/internal package.

Per discussion in the pull request, we'd like to avoid having an extra
dependency on a float comparison package. Instead, we copy the float compare
functions from the float comparison package.

The float comparison package we're choosing is this. The author of this
package has commented in the pull request and it looks like we have consensus
that this is the best option.
github.com/beorn7/floats

Signed-off-by: Seth Bunce <seth.bunce@gmail.com>
@sbunce
Copy link
Contributor Author

sbunce commented Sep 27, 2022

Sounds like there's consensus on avoiding the extra dependency. Reminds me of that go proverb, "a little copying is better than a little dependency".

I copied the contents of https://github.com/beorn7/floats to the prometheus/internal package. I made a few small changes.

  • I renamed "ε" to "epsilon". I feel like this improves readability for non-math people slightly. I can switch back to the original variable name if people want.
  • I modified the comment to reference the original repo.
  • I added variants which operate on slices of floats.

Couple points I anticipate there may be disagreement about. Calling these out.

  • I haven't copied over the tests. Seems like vendoring doesn't usually grab the tests, but this is a slightly different situation.
  • I copied over the float32 variant of things, even though we don't use it. Let me know if I should remove those.

Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

Looks good to me in general. Modulo location in file, see comment. Maintainers need to make the final call. @bwplotka @kakkoyun

prometheus/internal/difflib.go Outdated Show resolved Hide resolved
@beorn7
Copy link
Member

beorn7 commented Sep 28, 2022

I renamed "ε" to "epsilon".

I love the ε, but I am well aware that I'm a small minority here. (Insert lament about decline of classical education…)

@beorn7
Copy link
Member

beorn7 commented Sep 28, 2022

I copied over the float32 variant of things.

I think it is overwhelmingly likely that we'll never use the float32 here. I would prefer to remove this variant. It's easy to add later should we really need it, thanks to the reference back to the source repo.

All other points you made seem fine to me.

Thanks for your contribution and your patience.

@sbunce
Copy link
Contributor Author

sbunce commented Sep 28, 2022

You're welcome. I appreciate your feedback.

  • Removed the float32 variants. I bet you're right that this will never be used.
  • Moved the function to a new "almost_equal.go" file. I didn't realize the difflib file was a fork of a vendored dependency. Makes sense to not mix those. More properly put the copyright from the original repo at the top of the new file.

This change removes the float32 variant of the AlmostEqual funcs, that we will
likely never use. This change also relocates the function into a separate file
to avoid modifying a file that's a fork of another vendored package.

Signed-off-by: Seth Bunce <seth.bunce@gmail.com>
beorn7
beorn7 approved these changes Sep 29, 2022
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

Thanks again.

@bwplotka @kakkoyun please have a final look.

Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

LGTM :) Thanks for review @beorn7

@kakkoyun kakkoyun merged commit 4d54769 into prometheus:main Nov 7, 2022
6 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.

None yet

4 participants