Capture ResourcePath transfer_from exception in copy_files_for_distribution#108
Capture ResourcePath transfer_from exception in copy_files_for_distribution#108zhaoyuyoung wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #108 +/- ##
==========================================
+ Coverage 34.88% 37.06% +2.18%
==========================================
Files 13 13
Lines 1313 1330 +17
Branches 214 214
==========================================
+ Hits 458 493 +35
+ Misses 835 816 -19
- Partials 20 21 +1 ☔ View full report in Codecov by Sentry. |
| future_file_copy = {} | ||
| for src, trgt in files_to_copy.items(): | ||
| _LOG.debug("Staging %s to %s", src, trgt) | ||
| future = copy_executor.submit(trgt.transfer_from, src, transfer="copy") |
There was a problem hiding this comment.
I think this code is more complicated than it needs to be. If trgt and src are ResourcePath objects then ResourcePath.mtransfer can do all of this itself so you can delete most of this code.
There was a problem hiding this comment.
I'll check ResourcePath.mtransfer. This is a piece of legacy codes that started with IDF, so I guess it exists for a reason.
At the same time, I've found the reason why w_2026_12 failed to copy the quantum graph with the wrong protocol http:/ at RAL. It turns out the uri is treated as a local file other than the WebDAV one. So it's not a silent failure, but a successful transfer_from to the wrong destination.
There was a problem hiding this comment.
It existed before ResourcePath.mtransfer was written and which does exactly what this legacy code does.
There was a problem hiding this comment.
Thanks for showing me this very useful tool. I've changed to use ResourcePath.mtransfer and the exception it catches. Will ask for a review next week.
zhaoyuyoung
force-pushed
the
tickets/DM-54502
branch
2 times, most recently
from
9b46ccd to
3e517ff
Compare
| if future.result() is not None: | ||
| raise RuntimeError("Error of placing files to the distribution point") | ||
| _LOG.info("Staging %s to %s", src, trgt) | ||
| results = ResourcePath.mtransfer("copy", files_to_copy.items(), do_raise=False) |
There was a problem hiding this comment.
Are you sure you don't want to let it raise? You end up with a combo exception with all the failures in it whereas the code below is only reporting a single failure.
| raise RuntimeError( | ||
| f"Error of placing file to the distribution point: {trgt}" | ||
| ) from result.exception | ||
| if not trgt.exists(): |
There was a problem hiding this comment.
I'm not sure it's possible for the copy to succeed but the file to not be there.
There was a problem hiding this comment.
This was motivated by the initial "silent" failure we suspected. In w12 when the copy didn't show any exception, but pipetaskInit couldn't find the QG. If we don't need check the file exists, then we can turn ResourcePath.mtransfer raise to be true.
There was a problem hiding this comment.
If the copy succeeds the file will be there (unless something very strange has happened) so I don't think you need to explicitly need to check. Even if you want to explicitly check you should still let mtransfer raise.
There was a problem hiding this comment.
Done. Now let ResourcePath.mtransfer raise the grouped exceptions. Only two files to be transferred, so I leave the existence check there.
zhaoyuyoung
force-pushed
the
tickets/DM-54502
branch
from
3e517ff to
0893198
Compare
2 participants
Checklist
doc/changes