-
Notifications
You must be signed in to change notification settings - Fork 34
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
Dropulation execution errors #422
Comments
@drneavin , please provide reproduction procedure, i.e. the command line you invoked. |
Thanks @mschilli87! Would be great to have help testing this. @alecw , sorry for not posting this before. The command I'm running for v3.0.1 is:
For v3.0.0, the command that causes the positional error is:
Let me know if there's anything else that could help with troubleshooting. |
If I run AssignCellsToSamples --version using Drop-seq v. 3.0.0, I get
because of the function usage () {
[...]
} syntax not supported by POSIX shells. Explicitely calling bash $(which AssignCellsToSamples) --version results in the expected
I do remember fixing (or at least: trying to do so) this with a PR. I'll check and report back. @drneavin: Can you confirm that edit: I was talking about PR #398. |
I'm guessing that there is whitespace in one of the shell variables. Try wrapping all you variables in double quotes, i.e.
You'll probably still get an error, but the message should be more informative. |
I checked and as expected, my PR #398 fixed the syntax error when using v. 3.0.1: AssignCellsToSamples --version
So while the version is reported now, the script does not exit early but instead continues which triggers the 'missing argument' error. |
@drneavin: Could you just |
Hi both, For v3.0.0, @mschilli87, I think that makes sense. I don't have the singularity image with that version anymore but I don't see the same behaviour with 3.0.1 so I think your PR did fix that. For v3.0.1, unfortunately, the double quotes didn't help - I still get the same error. And when I echo the command I don't see anything that's clearly incorrect:
Note that I edited this to remove complete paths from our HPC for the barcode and the sample file. I also did a test where I set all the variables and ran using Drop-seq v2.5.4 from a previous singularity image which ran successfully and then changed just the singularity image to the one with Drop-seq v3.0.1 and resulted in the same positional argument error. This suggests to me that there isn't an issue with the variables unless the variable handling has been changed and it is more sensitive than before? Definitely open to suggestions and other interpretations. I also changed the order of the options (ie moved
This is the Demuxafy image with Drop-seq 3.0.1 installed if you wanted to test on your end to see if the result is the same or if you notice anything different about the setup or execution that are causing this problem:
|
I am still suspecting an unescaped parameter expansion since the argument in the error message starts with a |
Yes, I noticed that as well (the comma and the brace). But I double checked all my variables and none of them have a comma or a brace in them. Let me know if there's something else I can check? Would be great to get this working so I can include v3.0.1 in the next version of Demuxafy |
@drneavin: I would lime that very much so you definitely have me motivated to help. 😉 I had a look at the 3.0.1 shell scripts and I don't see any place where the @alecw: Did you update gradle between 2.5.4 and 3.0.1 or can we rule out a gradle regression? Else, this might be an error on the Java side for this specific command/class only. Also reproducing it with a direct call to Unfortunately I am not familiar enough with the gradle built system to |
Thanks @mschilli87! Glad we're both dedicated to getting this to work! And I think I've sorted it out 😄 . It seems that passing arguments as I'll leave this open so we can discuss and decide on the possible options for the parameter passing |
Hi @drneavin, I was wondering if that would solve the problem. We still use the old-school OPTION=value style that I won't go into the history of. A bad decision I made ~15 years ago. Anyway, I think there is another solution if you want to use the --OPTION style: If you put
|
I don't mind either way honestly - I'm happy to move to using the OPTION=value. I just want it to be easy and clear for other users as well. I'll update the docs on my end to reflect this so anyone using it through Demuxafy shouldn't run into any issues. But maybe in the next release the docs could be updated to use the old-school nomenclature in the help sections of each command? |
Unfortunately the argument parser is no longer something I control. I think the best solution would be to remove |
Ok, no worries. I think the good thing is if anyone comes across this error, they should find it when they search the issues. So should be an easy resolution for anyone else who makes the same mistake I made. Happy for you to close out this issue when you're satisfied |
Personally I like not having to set environment variables. But I get your point and either way is fine. I just agree with @drneavin (isn't it super late where you are?) that the documentation should match and the rest is resolved by this issue exisiting on the internet. |
I'm experiencing the same issue with
And the encountered error:
I also performed an For the second one, I'm using the command:
And the encountered error:
|
Hi @ltalignani , Have you tried the work-around I suggested in #422 (comment) ? -Alec |
Hi @alecw, I confirm that the |
Hello,
I'm having trouble getting dropulation in Drop-seq v3.0.0 and v3.0.1 to work. For v3.0.0 I receive the errors when I try to run:
and for v3.0.1 I get the following error:
I'm not sure if this is an issue on my end or is an issue that anyone trying to use these functions will come across. I'm using java v21 and building in a singularity image - Ubuntu 20.04. Happy to provide more details to help resolve the issue.
The text was updated successfully, but these errors were encountered: