ohsu-comp-bio / cwl-tes

cwl-tes submits your tasks to a TES server. Task submission is parallelized when possible.
Apache License 2.0
18 stars 28 forks source link

Recent FTP changes cause an error for glob outputs with * #42

Open aniewielska opened 4 years ago

aniewielska commented 4 years ago

Running this particular workflow fails for cwl-tes on gathering outputs of the step 2. (run_gwas). It fails for the version built from sources for this commit, but works fine for the recent version from pypi.

It looks like changes here, specifically appending FTP username and password to FTP URL cause problems with collecting outputs for globs with *.

Symptoms:

ERROR [job run_gwas] job error:
("Error collecting output for parameter 'logistic':\n../CWL/run_gwas.cwl:40:5: <urlopen error ftp error: error_perm('550 Failed to change directory.')>", {})

Debugging the code reveals that at some point in cwltool location with prepended user and password gets transformed into an invalid path and back to an invalid location.

Example invalid resulting location:

location': 'ftp://ftp-private.ebi.ac.uk/upload/ania//a02fb567-315e-482d-a6a3-82a2fe2ea1d7/output_a9346c71-7971-490d-937c-723d200a2eaf/37c-723d200a2eaf/ALL.chr21.integrated_phase1_v3.20101123.snps_indels_svs.genotypes_subsample.assoc.logistic'

where a correct one is:

location': 'ftp://ftp-private.ebi.ac.uk/upload/ania//a02fb567-315e-482d-a6a3-82a2fe2ea1d7/output_a9346c71-7971-490d-937c-723d200a2eaf/ALL.chr21.integrated_phase1_v3.20101123.snps_indels_svs.genotypes_subsample.assoc.logistic'
jvkersch commented 4 years ago

@aniewielska I'm not a maintainer, but I took a look at this since I've recently touched the ftp module. I think you are right that there is some weird interaction going on when paths are resolved, but I could not reproduce exactly.

At the same place as where you report your error, I ran into an issue with FtpFsAccess.listdir() returning incorrect paths. My issue is probably different from yours, but I think it is worth looking at listdir() anyway, since this is what determines the location parameter.

To understand a bit better what's going on, would you be able to run the following snippet? You'll have to set FTP_URL to something that makes sense for your configuration, e.g. (assuming you still have the files from when you ran the command) FTP_URL = ftp://ftp-private.ebi.ac.uk/upload/ania//a02fb567-315e-482d-a6a3-82a2fe2ea1d7/output_a9346c71-7971-490d-937c-723d200a2eaf/

The snippet will output some paths on the FTP server. I'm especially interested in whether these paths are correct or not (i.e. do they identify valid resources on the server). For context, different FTP servers seem to handle the NLST command differently and this (may) confuse listdir().

from cwl_tes.ftp import FtpFsAccess

FTP_URL = "ftp://bob:12345@localhost/A"

insecure = "localhost" in FTP_URL
fs_access = FtpFsAccess(FTP_URL, insecure=insecure)
print(fs_access.listdir(FTP_URL))
aniewielska commented 3 years ago

Hi @jvkersch Your snippet produces valid output - some actual files on the server. However, each entry is in the format: ftp://username:password@host/file, which I am not sure was expected. I think the problem is not in how FTP server responds, but in some operation on a path happening in cwltool, not taking into account, that the path is now prepended with username:password (the repeated part of the incorrect path correlates with the length of username+password).

I think the regression can be observed also by running one of the CWL conformance tests. It passes for this commit and fails for the current one.

Log:

Test 29 failed: 
Test that OutputBinding.glob is sorted as specified by POSIX
Returned non-zero
ERROR [job glob_test.cwl] job error:
("Error collecting output for parameter 'letters':\nv1.0/glob_test.cwl:11:3: <urlopen error ftp error: error_perm('550 Failed to change directory.')>", {})
WARNING Final process status is permanentFail
## An example run 
./run_test.sh RUNNER=/cwl-tes/bin/cwl-tes EXTRA="--tes http://some-tesk --remote-storage-url ftp://some-ftp" --tags=required -n29
jvkersch commented 3 years ago

@aniewielska Thanks for checking. I think you are right that the problem is due to a regression in how the path is handled when the username and password are included.