-
Notifications
You must be signed in to change notification settings - Fork 589
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
Replace File.createTempFile with IOUtils.createTempFile #6780
Conversation
lbergelson
commented
Aug 31, 2020
- This ensure that temp files are deleted on shutdown
- Fixes Gatk leaving behind config files #6771
@jamesemery mind taking a look at this? |
@lbergelson can you explain to me why this fixes the issue with tmp files? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh... I see. Our IOUtils.createTmpFile()
doesn't actually do the same thing as File.createTempFile()
it secretly creates a directory to house everything which happens to usually capture our indexes and secondary files. I almost think we should rename IOUtils.createTmpFile()
to IOUtils.createTmpFileHousedInTmpDir()
so its a little clearer. Also perhaps we should find a solution to scare people away from using the Files version in the future?
@jamesemery The big difference is that IOUtils marks the files for deletion on shutdown while Files doesn't. |
@jamesemery Also, since createTmpFile gives absolutely 0 information about where the temp file will be located it is completely fine to place each one in it's own unique directory, or on it's own moon or something. |
God that method is so much worse than i thought... We resort to hard coding common side outputs that are likely to show up and mark them for deletion in case we happen to produce one of them... I wonder if it is indeed possible to flag a directory for deletion and if we cant improve that method by secretly putting the file you ask for into a delete-on-exit folder so we don't have to guess about indexes and side outputs (that frankly don't look complete? What if my test has a tmp-fast dict/index those aren't included in the list of side outputs to flag for deletion). |
Deleting the whole folder is a good idea and we should do that instead of the targeted index indexes. |
@lbergelson do you want to try implementing the folder deletion in this PR or should we spin that approach off separately? |
@jamesemery No, lets do that as a separate pr. |
@@ -59,15 +60,8 @@ public void onTraversalStart() { | |||
metricOutput = outFile; // output file for summary metrics | |||
|
|||
// have to set the output file for variant eval; if not given a debug file to return the variant eval results | |||
// from, this will just be a temporary file that wil lbe deleted after the tool runs | |||
try { | |||
outFile = debugFile == null ? File.createTempFile("variant_eval" ,".txt") : debugFile; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we worry about this no longer catching the exception? Now this will err on this line throw new GATKException("Cannot create temp file: " + ex.getMessage(), ex);
which seems less specific but probably equally as informative? This is probably fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks fine, unless I missed something these were all trivial swaps mostly in the test files so I would say go ahead and merge this. I will go ahead and make a ticket to track the potential improvement to this code we discussed.
* This ensure that temp files are deleted on shutdown * Fixes #6771
3fb4820
to
de57310
Compare
rebased an rerunning tests. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #6780 +/- ##
===============================================
+ Coverage 86.643% 86.647% +0.004%
- Complexity 38964 38970 +6
===============================================
Files 2337 2337
Lines 182747 182763 +16
Branches 20067 20067
===============================================
+ Hits 158338 158359 +21
+ Misses 17366 17365 -1
+ Partials 7043 7039 -4
|