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

update fastp and fix linting and testing issues #5813

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

bgruening
Copy link
Member

fixes: #5812

FOR CONTRIBUTOR:

  • - I have read the CONTRIBUTING.md document and this tool is appropriate for the tools-iuc repo.
  • - License permits unrestricted use (educational + commercial)
  • - This PR adds a new tool or tool collection
  • - This PR updates an existing tool or tool collection
  • - This PR does something else (explain below)

@bgruening
Copy link
Member Author

Even our test data had this bug, I included a test that should fail.

@bernt-matthias bernt-matthias merged commit 61d055a into galaxyproject:main Mar 5, 2024
11 checks passed
@bgruening bgruening deleted the fasp_update branch March 5, 2024 08:32
@@ -25,19 +25,19 @@
#set $in2_name = re.sub('[^\w\-\s]', '_', str("%s_%s" % ($single_paired.paired_input.name, "R2"))) + $ext
#set out1 = $output_paired_coll.forward
#set out2 = $output_paired_coll.reverse
ln -s '$in1' '$in1_name' &&
ln -s '$in2' '$in2_name' &&
ln -sf '$in1' '$in1_name' &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that the fix to your issue, did you need the force for resubmitted jobs ? That seems like it would need a framework level fix, it doesn't seem like something we should fix within tools.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, just to be sure, resubmitted jobs are supposed to get a clean working dir, or?

Then the -f could only "help" with name collisions of the inputs (for which it might be better to fail).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, this was not the fix.

Resubmission from Galaxy gets a clean job_working_dir ... I think / hope / have seen it :) ... but job schedulers can also resubmit jobs. In this case we do not get a clean job dir I think.

Either way, this was not the fix, the update was the fix. The latest version is supposed to fail when the fastq files do not match up. I recommend updating this tool. For us, a few fastp jobs are blocking slots for days...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, eu uses htcondor retry/resubmit

but job schedulers can also resubmit jobs. In this case we do not get a clean job dir I think.

most likely not.

Maybe it would be cleaner to add a rm -rf working/* to the script referred to by environment_setup_file? The environment_setup_file is sourced in the job script (galaxy_xyz.sh), i.e. it would run with every submission.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fastp is running forever
3 participants