nils-braun / b2luigi

Task scheduling and batch running for basf2 jobs made simple
GNU General Public License v3.0
17 stars 11 forks source link

b2luigi/batch/processes/gbasf2.py: use '--failed_lfns' #132

Closed ArturAkh closed 3 years ago

ArturAkh commented 3 years ago

This pull request updates the behaviour of b2luigi in case of failed downloads from gbasf2 tasks and addresses the issue #130.

In the newest gbasf2 release v5r1p3, the option --failed_lfns becomes available to pass lfns of failed downloads to a text file. This allows to avoid parsing of stdout of the tool, which could lead to errors.

Since the format of the file created by --failed_lfns is exactly as required by --input_dslist in the subsequent download, no additional tests are needed. Therefore, all tests related to failed downloads are removed.

codecov-commenter commented 3 years ago

Codecov Report

Merging #132 (f170e23) into main (548e2c0) will decrease coverage by 0.23%. The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #132      +/-   ##
==========================================
- Coverage   56.54%   56.30%   -0.24%     
==========================================
  Files          23       23              
  Lines        1505     1506       +1     
==========================================
- Hits          851      848       -3     
- Misses        654      658       +4     
Impacted Files Coverage Δ
b2luigi/batch/processes/gbasf2.py 37.74% <0.00%> (-0.83%) :arrow_down:

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 548e2c0...f170e23. Read the comment docs.

meliache commented 3 years ago

Thanks, I took some time to look at this PR and looks good to me. Small reminder, I still prefer if you could create a separate named feature branch before opening the PR, this makes some git stuff less cumbersome :wink:

Thanks to you making this editable I did two minor changes which I thought don't require much discussions:

  1. I always think doing filepath.replace(...) is a bit dangerous, because it replaces all occurances of a string in the file path, e.g. if a super-directory would have failed_files as part of its name. Unlikely, but better just use os.path.splitext to add a suffix to the part of the filepath before the .txt
  2. Use with open(...) instead of open.readlines() directly to explicilty free resources instead of letting the garbage collector do that. I used the one-liner myself in the past, but now I saw in the internet that the with-version is more recommended.

I just pushed these directly to your main branch and think I will merge if it passes the tests.

ArturAkh commented 3 years ago

Hi @meliache thanks a lot for reviewing and fixing the pull request. I totally support your changes :)

And sorry for working on my main branch :D bad habbit ;)