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

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

Merged
merged 3 commits into from
Oct 11, 2021

Conversation

ArturAkh
Copy link

@ArturAkh ArturAkh commented Oct 8, 2021

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
Copy link

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%) ⬇️

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.

Doing "replace" on the file path for the current monitoring file might be
problematic, if the strings "failed_files" appears somewhere in the
filepath (e.g. a directory). In this case it is unlikely, but still, I prefer
using os.path.splitext to get the stem (part before the extension) of a filepath
and just append a suffix to that and re-append the extension.
In the past I myself used `open(...).readlines()`, but I found it is considered
better style to use `with` to explicitly free resources after opening the file.
In the first case the garbage collector would to it at some point.

Found this due to a pylint warning, see: https://stackoverflow.com/questions/67419064/r1732-consider-using-with-for-resource-allocating-operations-consider-using
@meliache meliache added enhancement New feature or request gbasf2 Concerns the gbasf2/grid b2luigi wrapper labels Oct 11, 2021
@meliache
Copy link
Collaborator

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 😉

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.

@meliache meliache merged commit 9c8bbc4 into nils-braun:main Oct 11, 2021
@ArturAkh
Copy link
Author

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 ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request gbasf2 Concerns the gbasf2/grid b2luigi wrapper
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replacing gb2_ds_get output parsing by --failed_lfns option to get file of failed LFNs
3 participants