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

B0FieldIdentifier: some characters should be replaced to set workflow name to avoid crashes in graph creation #295

Open
bpinsard opened this issue Nov 4, 2022 · 4 comments · Fixed by #434

Comments

@bpinsard
Copy link
Contributor

bpinsard commented Nov 4, 2022

FMRIPrep version 20.1.0
Not sure if that is in fMRIPrep or SDCFlows that the workflow name is set, but I get the following error if B0FieldIdentifier contains a - , and this will likely occur for other characters (I have no knowledge on dot files syntax though).

  File "/opt/conda/lib/python3.8/site-packages/fmriprep/cli/run.py", line 77, in main
    fmriprep_wf.write_graph(graph2use="colored", format="svg", simple_form=True)
  File "/opt/conda/lib/python3.8/site-packages/nipype/pipeline/engine/workflows.py", line 465, in write_graph
    outfname = format_dot(dotfilename, format=format)
  File "/opt/conda/lib/python3.8/site-packages/nipype/pipeline/engine/utils.py", line 1404, in format_dot
    formatted_dot, _ = _run_dot(dotfilename, format_ext=format)
  File "/opt/conda/lib/python3.8/site-packages/nipype/pipeline/engine/utils.py", line 1420, in _run_dot
    res = CommandLine(cmd, terminal_output="allatonce", resource_monitor=False).run()
  File "/opt/conda/lib/python3.8/site-packages/nipype/interfaces/base/core.py", line 428, in run
    runtime = self._run_interface(runtime)
  File "/opt/conda/lib/python3.8/site-packages/nipype/interfaces/base/core.py", line 822, in _run_interface
    self.raise_exception(runtime)
  File "/opt/conda/lib/python3.8/site-packages/nipype/interfaces/base/core.py", line 749, in raise_exception
    raise RuntimeError(
RuntimeError: Command:
dot -Tsvg -o"...../workdir/fmriprep_wf/graph.svg" ".../workdir/fmriprep_wf/graph.dot"
Standard output:

Standard error:
Error: ..../workdir/fmriprep_wf/graph.dot: syntax error in line 29 near '-'

dot file contains something like fmriprep_wf_single_subject_01_wf_fmap_preproc_wf_in_b0id-test on the offending line.

@bpinsard bpinsard changed the title B0FieldIdentifier: some characters should be replaced to set workflow to avoid crashes in graph creation B0FieldIdentifier: some characters should be replaced to set workflow name to avoid crashes in graph creation Nov 4, 2022
@bpinsard
Copy link
Contributor Author

bpinsard commented Nov 4, 2022

Replaced offending characters in my B0Field* tags and it works now, if that can help anyone encountering that issue.

@effigies
Copy link
Member

effigies commented Nov 4, 2022

Ah, thanks for that. I think we assumed it would be a camel case string.

@oesteban
Copy link
Member

We use the B0FieldIdentifier as an entity at the output (estimated fieldmaps). But the B0FieldIdentifier can definitely have hyphens, so we probably want to revise the workflow names and the datasinks.

@effigies
Copy link
Member

effigies commented Jun 5, 2024

This is turning out not to be resolvable purely in sdcflows. fMRIPrep at least (and thus probably nibabies and maybe dMRIPrep) use estimator.bids_id to both compare to B0FieldIdentifier and to look up workflow input/output spec names.

I think the fix, as @mgxd says in #434 (comment) is to make the sanitized ID available through the object API. We will then use that internally, and downstream tools need to transition.

If the sanitized ID replaces non-alphanumeric characters with _, then the workflows for currently working fieldmaps will be unchanged, and we could do this in a 2.8.x release. The workflows for non-working fieldmap identifiers will continue not to work, but at a different point of workflow construction.\

OTOH we would be introducing a new API feature, so it might make sense to call it 2.9.0, regardless.

WDYT? And do we care about getting this in for fMRIPrep 24.0, or should we leave it to the next round of releases?

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 a pull request may close this issue.

3 participants