-
Notifications
You must be signed in to change notification settings - Fork 597
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
ModelSegments integration test failures on newer Java 11 releases #8107
Comments
@samuelklee this ModelSegments issue we saw on Java 17 is now happening on current PRs in CI. We have a temporary solution for a short term, but can you or someone take a look? |
Thanks @cmnbroad, I’ll take a look today! Should hopefully just be a matter of updating expected results, but I see you mentioned some hints of Java 17 non-determinism in the linked PR. This seems worrisome, as the methods used by this tool should be deterministic and we saw no indications that they weren’t with Java 8… Will do some digging (if necessary), but if you have any examples, that would be much appreciated! |
@samuelklee I think the issue may be that we currently run integration tests on both Java 8 on the docker and on Java 11 on CI (though I'm not 100% sure that these specific tests run on both). I was able to update the expected results on the Java 17 branch, but that branch only runs on one version of Java. So updating the expected results may just move the problem to Java 8. Not sure. |
@cmnbroad Ah, thanks for reminding me of that. When will Java 8 be removed from the test suite? Looks like the numerical differences are big enough for some values that further relaxing the delta allowed for matching the expected results doesn't really make sense. I just opened #8111 which just does a hacky runtime check of the Java version and checks against the appropriate set of expected results accordingly. Seems to work locally, but let's see how the tests shake out. I also went ahead and unpinned the Java 11 version there. Whenever we get rid of Java 8, we can get rid of the "legacy" set of of expected results as well. I'll file a reminder issue if that PR ends up getting merged. Again, @tmelman might want to keep tabs on all of this. |
We can probably include the fix for this in the Java 17 branch, which is not that far from being merged, rather than trying to fix it now in the current hybrid Java 8/11 environment... |
See https://github.com/broadinstitute/gatk/actions/runs/3567534442/jobs/5995349354 for one example.
We originally saw these failures on the branch to move GATK to Java 17, but recently have started seeing the same failures on current PRs when running the tests on Java 11. It looks like this started happening when the CI env recently started resolving to Java 11.0.16.1., where these tests appear to always (?) fail, whereas previously the CI env was resolving to Java 11.0.11+9, where they pass. Although I haven't compared the results for all of the failed cases, I think the failure modes and bad values are the same on both Java 11 and Java 17.
We've temporarily pinned the CI environment to use 11.0.11+9 (see #8102) until we can get this resolved. I'd suspect the easiest way to reproduce the failures is to try running the tests using either Java 17 or Java 11.0.16.1.
The text was updated successfully, but these errors were encountered: