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

FIX: nest pathlib import in fix_multi_source_name #365

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

scott-huberty
Copy link
Contributor

@scott-huberty scott-huberty commented Jun 16, 2024

Per nipype.pipeline.engine.utils.evaluate_connect_function:

def evaluate_connect_function(function_source, args, first_arg):
    func = create_function_from_source(function_source)
    try:
        output_value = func(first_arg, *list(args))
    except NameError as e:
        if e.args[0].startswith("global name") and e.args[0].endswith("is not defined"):
            e.args = (
                e.args[0],
                (
                    "Due to engine constraints all imports have to be done "
                    "inside each function definition"
                ),
            )
        raise e
    return output_value

If I'm understanding this right I think we need to nest the pathlib.Path import within fix_multi_source_name.

Crash log that brought the issue to my attention
Node: nibabies_24_1_wf.single_subject_1011_sixmonth_wf.ds_report_about
Working directory: /scratch/nibabies_24_1_wf/single_subject_1011_sixmonth_wf/ds_report_about

Node inputs:

acquisition = <undefined>
atlas = <undefined>
base_directory = /out
ceagent = <undefined>
check_hdr = True
cohort = <undefined>
compress = <undefined>
data_dtype = <undefined>
datatype = figures
density = <undefined>
desc = about
direction = <undefined>
dismiss_entities = ['echo']
echo = <undefined>
extension = <undefined>
flip = <undefined>
fmap = <undefined>
fmapid = <undefined>
from = <undefined>
hemi = <undefined>
in_file = <undefined>
inv = <undefined>
label = <undefined>
meta_dict = <undefined>
modality = <undefined>
mode = <undefined>
model = <undefined>
mt = <undefined>
part = <undefined>
proc = <undefined>
reconstruction = <undefined>
recording = <undefined>
resolution = <undefined>
roi = <undefined>
run = <undefined>
scans = <undefined>
session = <undefined>
source_file = <undefined>
space = <undefined>
subject = <undefined>
subset = <undefined>
suffix = <undefined>
task = <undefined>
to = <undefined>

Traceback (most recent call last):
  File "/opt/conda/envs/nibabies/lib/python3.10/site-packages/nipype/pipeline/plugins/multiproc.py", line 344, in _send_procs_to_workers
    self.procs[jobid].run(updatehash=updatehash)
  File "/opt/conda/envs/nibabies/lib/python3.10/site-packages/nipype/pipeline/engine/nodes.py", line 497, in run
    self._get_hashval()
  File "/opt/conda/envs/nibabies/lib/python3.10/site-packages/nipype/pipeline/engine/nodes.py", line 548, in _get_hashval
    self._get_inputs()
  File "/opt/conda/envs/nibabies/lib/python3.10/site-packages/nipype/pipeline/engine/nodes.py", line 607, in _get_inputs
    output_value = evaluate_connect_function(
  File "/opt/conda/envs/nibabies/lib/python3.10/site-packages/nipype/pipeline/engine/utils.py", line 707, in evaluate_connect_function
    raise e
  File "/opt/conda/envs/nibabies/lib/python3.10/site-packages/nipype/pipeline/engine/utils.py", line 697, in evaluate_connect_function
    output_value = func(first_arg, *list(args))
  File "<string>", line 18, in fix_multi_source_name
NameError: name 'Path' is not defined


When creating this crashfile, the results file corresponding
to the node could not be found.

This PR fixed this issue on my side, but is there a unit test I can modify to check my understanding of the issue?

** FYI Looks like pathlib is still used by other functions in the nibabies.utils.misc module so I also kept the top level import of Path.. Let's see if that causes any flakes..

- I've kept the top level import because other functions still use Path. Let's see if that causes any flakes with the linter...
scott-huberty added a commit to scott-huberty/nipype that referenced this pull request Jun 17, 2024
- currently the if-statement in the function is very specific and ensures that the message both starts with 'global name' and 'is not defined'. nipreps/nibabies#365 shows that there is at least one example where this function does not catch a NameError that arises from a module-level import, where instead a less informative 'Variable' is not defined is thrown instead of saying 'hey, make sure modules/functions you call are imported at the function level'
scott-huberty added a commit to scott-huberty/nipype that referenced this pull request Jun 17, 2024
…ect_function more permissive

the if-statement in this function is very specific and ensures that the error message both starts with "global name" and ends with "is not defined" before raising the informative error about nested imports. nipreps/nibabies#365 gives one example where this if-statement is too specific, and doesn't catch a NameError that does actually arise from a module-level import.
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.

Thanks for the fix! This wasn't caught by the doctest since it isn't executed in context of a nipype wrapped function - the only ways I can think to better test these instances are:

  1. Wrap all functions used in connection statements as nipype Functions and test separately.
  2. Add a dataset to CI to test the pathway (in this case multi-anat)

Given that this is a quick bug fix and a large refactor of the codebase is nearing completion (#360), I'm inclined to just merging as is for the time being.

@mgxd mgxd merged commit f93aba7 into nipreps:master Jun 17, 2024
4 checks passed
@scott-huberty
Copy link
Contributor Author

Thanks for the fix! This wasn't caught by the doctest since it isn't executed in context of a nipype wrapped function - the only ways I can think to better test these instances are:

  1. Wrap all functions used in connection statements as nipype Functions and test separately.
  2. Add a dataset to CI to test the pathway (in this case multi-anat)

Given that this is a quick bug fix and a large refactor of the codebase is nearing completion (#360), I'm inclined to just merging as is for the time being.

Ah, nice! sure, since it just duplicates an import it doesn't risk breaking existing code, so up to you!

@scott-huberty scott-huberty deleted the import_pathlib branch June 17, 2024 04:49
@scott-huberty scott-huberty mentioned this pull request Aug 14, 2024
mgxd added a commit that referenced this pull request Aug 30, 2024
24.0.0 (August 29, 2024)

This major release includes a substantial refactoring of the pipeline.

One key addition is the addition of the `--level` flag, which can take the arguments minimal, resampling or full. The default is full, which should produce nearly the same results as previous versions. minimal will produce only the minimum necessary to deterministically generate the remaining derivatives. resampling will produce some additional derivatives, intended to simplify resampling with other tools.

The `--derivatives` flag was altered to take arguments in the form `name=/path/to/dir`.
For each directory provided, if a derivative is found - it will be used instead of computing it from scratch. If a derivative is not found, NiBabies will compute it and proceed as usual.

Taken together, these features can allow a dataset provider to run a minimal NiBabies run, targeting many output spaces, while a user can then run a `--derivatives` run to generate additional derivatives in only the output spaces they need. Another use case is to provide an precomputed derivative to override the default NiBabies behavior, enabling easier workarounds for bugs or experimentation with alternatives.

Another new feature is a dynamic anatomical reference, which is set based on surface reconstruction method or through the `--reference-anatomical` flag. Previously, T1w was the default output space. Now, the reference anatomical is determined based on the surface reconstruction method.

Additionally, minor adjustments have been made to MCRIBS surface reconstruction to address failure rates. This is still an on-going investigation, but preliminary results look promising.

This release resolves a number of issues with fieldmaps inducing distortions during correction. Phase difference and direct fieldmaps are now masked correctly, preventing the overestimation of distortions outside the brain. Additionally, we now implement Jacobian weighting during unwarping, which corrects for compression and expansion effects on signal intensity. To disable Jacobian weighting, add `fmap-jacobian` to the `--ignore` argument.

Finally, a new resampling method has been added, to better account for susceptibility distortion and motion in a single shot resampling to a volumetric target space. We anticipate extending this to surface targets in the future.

  * FIX: nest pathlib import in fix_multi_source_name (#365)
  * FIX: Avoid retrieving multiple templates from latest TF (#353)
  * FIX: Raise informative error if no t1w or t2w found (#347)
  * FIX: Easier pyenv usage (#342)
  * FIX: Catch nonexistent derivatives, clean up subworkflow logic (#336)
  * FIX: Use fsLR reg sphere for MCRIBS morphometrics resampling (#334)
  * FIX: T2star map MNI scaling (#320)

  * ENH: Alter outputs when MCRIBS reconstruction is used (#329)
  * ENH: Use nireports for Report generation + add reportlet per reconstruction (#328)
  * ENH: better repr for Derivatives class (#351)

  * RF: Move to fit/apply workflow (#360)
  * RF: Replace `resource_filename` with `load_data` (#345)

  * MAINT: Bump urllib3 from 2.0.3 to 2.0.7 (#319)
  * MAINT: Raise minimum to 3.10, bump actions (#337)
  * MAINT: Bump pillow from 9.5.0 to 10.0.1 (#317)
  * MAINT: Update to latest migas API (#326)

  * DOC: Use correct argument flag (#338)
  * DOC: Move to new theme, add outputs description (#383)
effigies pushed a commit to scott-huberty/nipype that referenced this pull request Oct 6, 2024
…ect_function more permissive

the if-statement in this function is very specific and ensures that the error message both starts with "global name" and ends with "is not defined" before raising the informative error about nested imports. nipreps/nibabies#365 gives one example where this if-statement is too specific, and doesn't catch a NameError that does actually arise from a module-level import.
effigies pushed a commit to scott-huberty/nipype that referenced this pull request Oct 10, 2024
…ect_function more permissive

the if-statement in this function is very specific and ensures that the error message both starts with "global name" and ends with "is not defined" before raising the informative error about nested imports. nipreps/nibabies#365 gives one example where this if-statement is too specific, and doesn't catch a NameError that does actually arise from a module-level import.
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