from importlib import reload for Python 3 by cclauss · Pull Request #675 · joblib/joblib

cclauss · 2018-05-09T17:04:29Z

reload() was moved into importlib in Python 3 because it was being overuse and led issues that were to difficult-to-debug.

flake8 testing of https://github.com/joblib/joblib on Python 3.6.3

$ flake8 . --count --select=E901,E999,F821,F822,F823 --show-source --statistics

./joblib/test/test_memory.py:599:36: F821 undefined name 'func_with_kwonly_args'
        func_cached = memory.cache(func_with_kwonly_args)
                                   ^
./joblib/test/test_memory.py:620:36: F821 undefined name 'func_with_kwonly_args'
        func_cached = memory.cache(func_with_kwonly_args, ignore=['kw2'])
                                   ^
./joblib/test/test_memory.py:626:36: F821 undefined name 'func_with_signature'
        func_cached = memory.cache(func_with_signature)
                                   ^
./joblib/test/test_parallel.py:490:5: F821 undefined name 'reload'
    reload(module)
    ^
4     F821 undefined name 'func_with_kwonly_args'
4

__reload()__ was moved into __importlib__ in Python 3 because it was being overuse and led issues that were to difficult-to-debug. flake8 testing of https://github.com/joblib/joblib on Python 3.6.3 $ __flake8 . --count --select=E901,E999,F821,F822,F823 --show-source --statistics__ ``` ./joblib/test/test_memory.py:599:36: F821 undefined name 'func_with_kwonly_args' func_cached = memory.cache(func_with_kwonly_args) ^ ./joblib/test/test_memory.py:620:36: F821 undefined name 'func_with_kwonly_args' func_cached = memory.cache(func_with_kwonly_args, ignore=['kw2']) ^ ./joblib/test/test_memory.py:626:36: F821 undefined name 'func_with_signature' func_cached = memory.cache(func_with_signature) ^ ./joblib/test/test_parallel.py:490:5: F821 undefined name 'reload' reload(module) ^ 4 F821 undefined name 'func_with_kwonly_args' 4 ```

codecov · 2018-05-09T17:15:43Z

Codecov Report

Merging #675 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #675      +/-   ##
==========================================
+ Coverage    95.1%   95.14%   +0.03%     
==========================================
  Files          40       40              
  Lines        5746     5750       +4     
==========================================
+ Hits         5465     5471       +6     
+ Misses        281      279       -2

Impacted Files	Coverage Δ
joblib/test/test_parallel.py	`96.64% <100%> (+0.02%)`	⬆️
joblib/_parallel_backends.py	`96% <0%> (ø)`	⬆️
joblib/test/test_memory.py	`98.16% <0%> (+0.36%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb027ab...9038be9. Read the comment docs.

jakirkham · 2018-06-23T04:43:21Z

Guessing this refers to this line? Confusing why it wasn't causing issues before. Maybe I'm overlooking something?

cclauss · 2018-06-23T05:40:01Z

The comment says that the reload() line should raise an error but perhaps currently on Python 3 that is NameError because reload() is undefined.

ogrisel · 2018-06-25T06:31:16Z

Yes, it would be good to investigate why it was not causing a crash (maybe a bare except somewhere up in the stack?).

In any case, it would be great to add a non-regression test to show that the call to reload actually work if not too cumbersome to add.

lesteve · 2018-06-25T08:00:39Z

IIRC test_multiple_spawning actually does not actually test anything because _reload_joblib always raises an ImportError on the __import__ line (don't remember why but basically the imported module string is not constructed correctly or something like this).

joblib/joblib/test/test_parallel.py

Lines 543 to 551 in bb027ab

    
           def test_multiple_spawning(): 
        
               # Test that attempting to launch a new Python after spawned 
        
               # subprocesses will raise an error, to avoid infinite loops on 
        
               # systems that do not support fork 
        
               if not int(os.environ.get('JOBLIB_MULTIPROCESSING', 1)): 
        
                   raise SkipTest() 
        
               with raises(ImportError): 
        
                   Parallel(n_jobs=2, pre_dispatch='all')( 
        
                       [delayed(_reload_joblib)() for i in range(10)])

I think the original intent of the test was to test these lines but it hasn't fulfilled its purposes for a while:

joblib/joblib/_parallel_backends.py

Lines 428 to 435 in bb027ab

    
           raise ImportError( 
        
               '[joblib] Attempting to do parallel computing ' 
        
               'without protecting your import on a system that does ' 
        
               'not support forking. To use parallel-computing in a ' 
        
               'script, you must protect your main loop using "if ' 
        
               "__name__ == '__main__'" 
        
               '". Please see the joblib documentation on Parallel ' 
        
               'for more information')

I think it's fine to merge this PR, but finding a replacement test for test_multiple_spawning still remains to be done.

* tag '0.12.1': Release 0.12.1 fix kwonlydefaults key error in filter_args (joblib#715) MNT fix some "undefined name" flake8 warnings (joblib#713). from importlib import reload for Python 3 (joblib#675) MTN vendor loky2.1.4 (joblib#708)

* releases: (121 commits) Release 0.12.1 fix kwonlydefaults key error in filter_args (joblib#715) MNT fix some "undefined name" flake8 warnings (joblib#713). from importlib import reload for Python 3 (joblib#675) MTN vendor loky2.1.4 (joblib#708) Release 0.12 typo typo typo ENH add initializer limiting n_threads for C-libs (joblib#701) DOC better parallel docstring (joblib#704) [MRG] Nested parallel call thread bomb mitigation (joblib#700) MTN vendor loky2.1.3 (joblib#699) Make it possible to configure the reusable executor workers timeout (joblib#698) MAINT increase timeouts to make test more robust on travis DOC: use the .joblib extension instead of .pkl (joblib#697) [MRG] Fix exception handling in nested parallel calls (joblib#696) Fix skip test lz4 not installed (joblib#695) [MRG] numpy_pickle: several enhancements (joblib#626) Introduce Parallel.__call__ backend callbacks (joblib#689) ...

Merge branch 'master' into patch-1

9038be9

ogrisel merged commit 9028693 into joblib:master Jul 2, 2018

ogrisel mentioned this pull request Jul 2, 2018

test_multiple_spawning is not testing what it's supposed to test #710

Closed

cclauss deleted the patch-1 branch July 2, 2018 07:50

NicolasHug mentioned this pull request Mar 29, 2020

[MRG] Removed dead code #1027

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

from importlib import reload for Python 3#675

from importlib import reload for Python 3#675
ogrisel merged 2 commits intojoblib:masterfrom
cclauss:patch-1

cclauss commented May 9, 2018

Uh oh!

codecov bot commented May 9, 2018 •

edited

Loading

Uh oh!

jakirkham commented Jun 23, 2018

Uh oh!

cclauss commented Jun 23, 2018

Uh oh!

ogrisel commented Jun 25, 2018

Uh oh!

lesteve commented Jun 25, 2018 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

cclauss commented May 9, 2018

Uh oh!

codecov bot commented May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jakirkham commented Jun 23, 2018

Uh oh!

cclauss commented Jun 23, 2018

Uh oh!

ogrisel commented Jun 25, 2018

Uh oh!

lesteve commented Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented May 9, 2018 •

edited

Loading

lesteve commented Jun 25, 2018 •

edited

Loading