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

Added skip_citation_process flag to skip processing citation.md #1876

Merged
merged 3 commits into from Nov 25, 2019

Conversation

soichih
Copy link
Contributor

@soichih soichih commented Nov 22, 2019

Changes proposed in this pull request

This PR workarounds the issue of pandoc running out of vmem at the end of the fmriprep pipeline which then terminates the workflow on our HPC environement. #1870

Here is Neurostart discussion about this issue.

https://neurostars.org/t/large-virtual-memory-allocations-in-fmriprep-on-pbs-system/4665/8

Our workflow doesn't just run a single fmriprep, so if fmriprep fails at the end, it will kill the entire workflow. For us, it's important that fmriprep survives the post-processing. The memory issue with pando has been a known issue since 2016 according to this thread (jgm/pandoc#3169) ..

causes jobs to fail due to pandoc using too much vmem
@welcome
Copy link

welcome bot commented Nov 22, 2019

Thanks for opening this pull request! It looks like this is your first time contributing to fMRIPrep. 😄
We invite you to list yourself as an fMRIPrep contributor. To learn more about what that entails and how we credit our contributors, please check out the contributing guidelines. If your name is not already on the list, please insert it, in alphabetical order, into the contributors.json file. Example:

   "name": "Contributor, New FMRIPrep",
   "affiliation": "Department of fMRI prep'ing, Open Science Made-Up University",
   "orcid": "<your id>"
}, ```

Of course, if you want to opt-out this time there is no problem at all with adding your name later. You will be always welcome to add it in the future whenever you feel it should be listed.

@auto-comment

This comment has been minimized.

@soichih soichih changed the title Added skip_citation_process flag to skip processing citation.md which Added skip_citation_process flag to skip processing citation.md Nov 22, 2019
Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Thanks. This should be moved to a different argument group.

I also wonder if there's a clearer name, but my brain isn't coming up with anything better. Something about "process" feels vague, and once it exists, it'll be annoying to change. So in case someone has a better idea, now would be a good time to change it.

@@ -73,6 +73,9 @@ def get_parser():
g_bids.add_argument('--skip_bids_validation', '--skip-bids-validation', action='store_true',
default=False,
help='assume the input dataset is BIDS compliant and skip the validation')
g_bids.add_argument('--skip_citation_process', '--skip-citation-process', action='store_true',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
g_bids.add_argument('--skip_citation_process', '--skip-citation-process', action='store_true',
g_bids.add_argument('--skip-citation-process', action='store_true',

Usually the _ one is there for backwards compatibility with old submission scripts. - is preferred, so we can use it on new options.

And this should go in g_perfm, not g_bids.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will update the PR. Testing the update now.

@faskowit
Copy link

could 'skip-pandoc' or 'no-pandoc' be a more direct/intuitive flag name? Given the brief help text, I think it would still be pretty clear to users.

@emdupre
Copy link
Collaborator

emdupre commented Nov 23, 2019

could 'skip-pandoc' or 'no-pandoc' be a more direct/intuitive flag name?

I'm hesitant to mention pandoc in the argument, since I imagine a decent number of users won't be familiar with it.

What about --md-only-boilerplate ?

@pep8speaks
Copy link

pep8speaks commented Nov 24, 2019

Hello @soichih! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-11-24 21:50:23 UTC

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

LGTM. Any further comments?

@oesteban oesteban merged commit 7bc943e into nipreps:master Nov 25, 2019
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.

None yet

6 participants