Skip to content

Conversation

@effigies
Copy link
Member

Inspired by nipreps/nibabies#82.

workflow.connect() is tab-shifted but otherwise unchanged.

@effigies effigies requested a review from mgxd June 30, 2021 13:02
Copy link
Collaborator

@mgxd mgxd left a comment

Choose a reason for hiding this comment

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

Should we generalize this to the whole workflow building process? i.e.

retval["workflow"] = init_fmriprep_wf()

I know we are building within a subprocess, which might make this error reporting too complicated, but it would be nice to handle this in the case of every connect statement

@effigies
Copy link
Member Author

The nice thing about where I put it is that we have the specific file name as well as the exception, so we can be pretty precise. Wrapping the line you suggest would lose that.

Besides, what are the other cases where there might be a collision? The subject ID is in the subject workflow name, and all the entities are in the functional workflow name, so AFAIK the .nii/.nii.gz duplication is the only possibility. If we see others, it would be good to get them, but a quick survey of old issues showed one pybids bug, and a bunch of this.

@mgxd
Copy link
Collaborator

mgxd commented Jun 30, 2021

Sounds good - I agree it does give a better message there. If the duplication error ever comes up again we can revisit this - but I'm fine with the changes as is

@effigies
Copy link
Member Author

Cool. Guess I'll wait on the nibabies issue, to see if there is some other way that we're hitting this.

@mgxd
Copy link
Collaborator

mgxd commented Jun 30, 2021

The nibabies error was due to an internal bug, merging.

@mgxd mgxd merged commit e31573f into nipreps:maint/20.2.x Jun 30, 2021
@effigies effigies deleted the fix/detect_duplicate_subworkflows branch July 1, 2021 00:31
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