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

Disable P3M benchmark test #3347

Closed
wants to merge 1 commit into from

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented Nov 29, 2019

Closes #2924

The benchmark test is fragile and breaks regularly in CI. The P3M
benchmark itself is not useful at the moment, as the P3M tuning
method is non-deterministic. Multiple attempts at fixing the tuning
function did not resolve the issue.
@jngrad jngrad added the DevOps label Nov 29, 2019
@jngrad jngrad requested a review from fweik November 29, 2019 13:00
@codecov
Copy link

codecov bot commented Nov 29, 2019

Codecov Report

Merging #3347 into python will increase coverage by <1%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           python   #3347    +/-   ##
=======================================
+ Coverage      86%     86%   +<1%     
=======================================
  Files         538     538            
  Lines       25348   25348            
=======================================
+ Hits        21830   21831     +1     
+ Misses       3518    3517     -1
Impacted Files Coverage Δ
src/core/particle_data.cpp 97% <0%> (-1%) ⬇️
src/core/electrostatics_magnetostatics/p3m.cpp 85% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e47dbe4...59a8b09. Read the comment docs.

@fweik
Copy link
Contributor

fweik commented Nov 29, 2019

Hm, if you just comment it out, than it will decay and stop working. I think it would be better to either make it deterministic by fixing the parameters or remove it altogether.

@jngrad
Copy link
Member Author

jngrad commented Nov 29, 2019

I think it would be better to either make it deterministic by fixing the parameters

All the information I got from coworkers, from code introspection and from debugging sessions where parameters and random seeds are fixed, are consistent with a non-deterministic P3M tuning algorithm, for a continuous period of 6 months. More specifically, the bisection uses a heuristic that depends on the exact timing of all previous iterations, and these timings are affected by all other threads running on the same CI machine. I don't see what you have in mind here. Didn't you once suggest we should re-implement the P3M tuning in Python with a different algorithm to make it more deterministic?

it will decay and stop working

True. Let's wait until #3326 is merged, then we can disable the benchmark tests only on the subset of CI jobs where it fails.

@jngrad jngrad closed this Nov 29, 2019
@fweik
Copy link
Contributor

fweik commented Nov 29, 2019

I meant fixing the P3M parameters instead of tuning them. The tuning as it stands is non-deterministic because it depends on the (uncontrolled) run time.

@jngrad
Copy link
Member Author

jngrad commented Nov 29, 2019

That could work on a single machine. Getting the P3M parameters once and re-using them for all other myconfig files would make the benchmark actually useful. But can we come up with P3M parameters that would work on any machine?

@jngrad jngrad deleted the disable-p3m-benchmark-test branch May 27, 2022 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

P3M benchmark randomly fails CI
2 participants