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

Check mount paths exist #72

Merged
merged 9 commits into from
May 11, 2023
Merged

Conversation

ManavalanG
Copy link
Member

@ManavalanG ManavalanG commented May 9, 2023

Pull request

  • Adds a verification step in the CLI wrapper script to check if the file/dirpaths to be mounted to singularity already exist as expected (Verify singularity mount paths exist #71). This would help debugging easier in cases where paths don't exist as expected.
  • Resolves dirpaths of datasets in the workflow config file to obtain their full path

Please fill in the checklist below and comment as needed.

  • Was code modified? Briefly describe. Yes. See description.
  • Was documentation modified? Briefly describe. No.
  • Is this a bug-fix? Briefly describe. No.
  • Is this a feature addition? Briefly describe. Yes. See description.
  • Did you modify QuaC-Watch config file? If so, did you modify multiqc template
    configs/multiqc_config_template.jinja2 and script src/quac_watch/create_mutliqc_configs.py as necessary? No.
  • Did you perform system-level testing manually as described in master readme doc? Did it pass completely? If not why?
  • Updated Changelog.md file with change logs in recommended format?

Anything else reviewer should know?

@ManavalanG ManavalanG requested a review from wilkb777 May 9, 2023 17:01
@ManavalanG ManavalanG marked this pull request as draft May 9, 2023 17:01
Copy link
Member

@wilkb777 wilkb777 left a comment

Choose a reason for hiding this comment

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

I think it'd be better to change the check_path_exists function to take a list of paths and then check all entries in the list and return a list of missing paths all in one shot. Otherwise you'll have to do multiple runs of QuaC to catch all the missing paths with the current setup.

The extra benefit from this will also be that you can reduce the call of the function in read_workflow_config to a single call to check all the mount paths and files in one shot.

@ManavalanG ManavalanG marked this pull request as ready for review May 11, 2023 02:53
@ManavalanG
Copy link
Member Author

ManavalanG commented May 11, 2023

@wilkb777 Good point. I have now refactored to check mount paths all at once. Note that earlier I used to verify if the datasets specified in the workflow config file existed, but now the scope is restricted to just check the directories that will be mounted to singularity. That is, pipeline may still fail if the specified filepath doesn't exist under those directories. I think this is okay though.

Additionally, I have now refactored to resolve the dirpaths in the workflow config file; so no more partial paths. This should also take symlinks. [Updated MR description to reflect this change].

Checked system testing after these changes, and it was successful.

Copy link
Member

@wilkb777 wilkb777 left a comment

Choose a reason for hiding this comment

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

Excellent, that's a great change along with the path existence assertion. LGTM!

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.

2 participants