More helpful warnings when publish fails to create parent directory #4241

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

Jump to bottom

Open

robsyme wants to merge 1 commit into nextflow-io:master

from robsyme:warn-when-cannot-create-publishdir

Open

More helpful warnings when publish fails to create parent directory #4241

robsyme wants to merge 1 commit into nextflow-io:master from robsyme:warn-when-cannot-create-publishdir

Conversation

robsyme

Copy link

Collaborator

@robsyme robsyme commented Aug 30, 2023

At the moment, if publishing a file fails because of permissions, Nextflow will return an error indicating the Access Denied exception, but will not indicate which file was the problem.

This commit adds the file name to the error message, so that the user can determine from the logs alone which file path was the problem. Catching this exception also triggers the safeProcessFile catch block which also gives the helpful message saying "Failed to publish file". It's not entirely clear to me why this catch block does not catch the Access Denied exception being thrown inside the makeDirs method.

@robsyme


 At the moment, if publishing a file fails because of permissions,

3f6798d

Nextflow will return an error indicating the Access Denied exception,
but will not indicate which file was the problem.
This commit adds the file name to the error message, so that the user
can determine from the logs alone which file path was the problem.
Catching this exception also triggers the safeProcessFile catch block
which also gives the helpful message saying "Failed to publish file".
It's not entirely clear to me why this catch block does not catch the
Access Denied exception being thrown inside the `makeDirs` method.
Signed-off-by: Rob Syme <rob.syme@gmail.com>

@netlify

Copy link

netlify bot commented Aug 30, 2023 •

edited

Loading

✅ Deploy Preview for nextflow-docs-staging canceled.

Name	Link
🔨 Latest commit	`3f6798d`
🔍 Latest deploy log	https://app.netlify.com/sites/nextflow-docs-staging/deploys/64efa059bfd07c0008a9dc03

@bentsherman bentsherman added the error-improvements label

Aug 30, 2023

@robsyme

Copy link

Collaborator Author

robsyme commented Aug 31, 2023

I'd like to note that I'm very unclear why this change is necessary. From my reading of the code, safeProcessFile calls processFile which calls makeDirs, which throws the exception:

safeProcessFile
 | processFile
 | makeDirs -> throws the Exception

But I'm very unclear why this exception isn't caught by the try block

protected void safeProcessFile(Path source, Path target) {
 try {
 processFile(source, target)
 }
 catch( Throwable e ) {
 log.warn "Failed to publish file: ${source.toUriString()}; to: ${target.toUriString()} [${mode.toString().toLowerCase()}] -- See log file for details", e
 if( NF.strictMode || failOnError){
 final session = Global.session as Session
 session?.abort(e)
 }
 }
}

Ben/Paolo, can you explain why the try block in the safeProcessFile method isn't catching the Exception?

@marcodelapierre marcodelapierre requested a review from bentsherman

November 22, 2023 03:31

@bentsherman bentsherman changed the title ~~(削除) More helpful warnings when publish fails to create parent directory. (削除ここまで)~~ (追記) More helpful warnings when publish fails to create parent directory (追記ここまで)

Nov 22, 2023

@bentsherman

Copy link

Member

bentsherman commented Nov 22, 2023

Sorry for letting this one fall through. Did you confirm that makeDirs was called via processFile ? Because it is also called via apply0 > createPublishDirs.

If that's not it, then I don't know why the exception wouldn't be caught. But in any case it would probably be helpful to have this warning directly in makeDirs so that it is properly logged