FIX: evaluate_connect_function should raise error on un-nested imports

[scott-huberty]

Summary

if I'm understanding evaluate_connect_function correctly, the intent of the if-clause is to catch errors that arise from calling libraries/modules/functions that were imported at the module level (as opposed to nesting the imports at the function level), and raises a more informative error for those cases.

I think nipreps/nibabies#365 gives one example where the if-clause should catch such an error, but doesn't, because the error doesn't start with "global name". So I think it should be okay to make the if-clause more permissive, i.e. just check if the error message ends with "is not defined", then return the extra-informative error message to the user.

What do you think?

[effigies]

I think this is probably a change in Python's error messages, which should not be relied upon as an API. We should probably just do:

    except NameError as e:
        e.args = (
            *e.args,
            "Due to engine constraints all imports have to be done inside each function definition"
        )
        raise e

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.85%. Comparing base (fffc1c2) to head (df241b2).
Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
nipype/pipeline/engine/utils.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           master    #3655     +/-   ##
=========================================
  Coverage   70.85%   70.85%             
=========================================
  Files        1277     1277             
  Lines       59118    59116      -2     
  Branches     9803     8596   -1207     
=========================================
- Hits        41888    41887      -1     
- Misses      16061    16084     +23     
+ Partials     1169     1145     -24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[scott-huberty]

I think this is probably a change in Python's error messages, which should not be relied upon as an API. We should probably just do:

    except NameError as e:
        e.args = (
            *e.args,
            "Due to engine constraints all imports have to be done inside each function definition"
        )
        raise e

Thanks @effigies , I think that is a nice idea (much simpler).

I incorporated this suggestion in 4e0b503 , except that I just explicitly raise a NameError, and via string formatting I include the original message plus our extra info (as opposed to overwriting the original error message). Since it looks like Nipype supports 3.7 or later I think this should be okay, but let me know if you disagree!