BaseRecalibratorSpark fails on a cluster due to system classloader issue #5979
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… find a resource.
|
@LeeTL1220 would you mind taking a look at this please (it came about from changes in #5941). |
|
Stack trace from failed job: |
Codecov Report
@@ Coverage Diff @@
## master #5979 +/- ##
===========================================
Coverage 86.929% 86.929%
Complexity 32721 32721
===========================================
Files 2013 2013
Lines 151306 151306
Branches 16610 16610
===========================================
Hits 131529 131529
Misses 13720 13720
Partials 6057 6057
|
LeeTL1220
approved these changes
Jun 4, 2019
| if (systemResourceAsStream == null) { | ||
| throw new GATKException("Null value when trying to read system resource. Cannot find: " + resourcePath); | ||
| if (resourceAsStream == null) { | ||
| resourceAsStream = Resource.class.getClassLoader().getResourceAsStream(resourcePath); |
There was a problem hiding this comment.
Can't we just use this statement above and eliminate the if statement?
I.e. replace InputStream resourceAsStream = ClassLoader.getSystemResourceAsStream(resourcePath); with
InputStream resourceAsStream = Resource.class.getClassLoader().getResourceAsStream(resourcePath);
I would think that the ClassLoader would be equivalent to Resource.class.getClassLoader()
Apologies if I am forgetting something....
There was a problem hiding this comment.
I think you're right. A test seems to be failing now, but I'm not sure it's related. I'll investigate.
|
|
||
| if (systemResourceAsStream == null) { | ||
| throw new GATKException("Null value when trying to read system resource. Cannot find: " + resourcePath); | ||
| if (resourceAsStream == null) { |
There was a problem hiding this comment.
Can you add a one-line comment why we need this? "For Spark, etc etc"
|
Thank you for taking a look @LeeTL1220. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The problem is that Spark executors can't rely on the system classloader to load resources. This change falls back to the current classloader if the resource can't be loaded from the system classloader. I've tested it successfully on a cluster.