Support to upload modules from zip files #886

bmaisonn · 2017-02-20T22:41:17Z

Fix issue (#865).

…cumentation

mrocklin

Generally this looks pretty good to me. A few comments.

mrocklin · 2017-02-20T23:52:36Z

distributed/tests/test_client.py

+            if os.path.exists('myfile.zip'):
+                os.remove('myfile.zip')
+
+    sleep(1)  # TODO:  why is this necessary?


What happens if this is not present?

The one second resolution may depend on the filesystem. Try importlib.invalidate_caches() instead. (see https://docs.python.org/3/library/importlib.html#importlib.invalidate_caches ). It isn't needed on Python 2.

There's no case where we call invalidate_cache in the upload_file method.
Should I add it in all cases ?
In the same way the sleep call is in all the upload_file tests.
Should I remove it from all the tests ?

if ext in ('.py', '.pyc'): logger.info("Reload module %s from .py file", name) name = name.split('-')[0] reload(import_module(name)) if ext == '.egg': import pkg_resources sys.path.append(out_filename) pkgs = pkg_resources.find_distributions(out_filename) for pkg in pkgs: logger.info("Load module %s from egg", pkg.project_name) reload(import_module(pkg.project_name)) if not pkgs: logger.warning("Found no packages in egg file") if ext == '.zip': logger.info("Reload module %s from .zip file", name) if out_filename not in sys.path: sys.path.insert(0, out_filename) invalidate_caches() reload(import_module(name))

You should at least try it and see it if works, yes :-)

I added the invalid_caches and commonalized some code:

try: name, ext = os.path.splitext(filename) names_to_import = [] if ext in ('.py', '.pyc'): names_to_import.append(name) if ext in ('.egg', '.zip'): if out_filename not in sys.path: sys.path.insert(0, out_filename) if ext == '.egg': import pkg_resources pkgs = pkg_resources.find_distributions(out_filename) for pkg in pkgs: names_to_import.append(pkg.project_name) elif ext == '.zip': names_to_import.append(name) if not names_to_import: logger.warning("Found nothing to import from %s", filename) else: invalidate_caches() for name in names_to_import: logger.info("Reload module %s from %s file", name, ext) reload(import_module(name))

I removed the sleep in my test case and it works fine (But it also works without the call to invalidate_caches).
However the test still fails in the 'test_upload_file' case which only uploads .py or .pyc files

maybe I found the answer here :
http://stackoverflow.com/questions/8122734/pythons-imp-reload-function-is-not-working

That may be the case indeed. You may want to try to remove the cached bytecode file. On Python 3, see https://docs.python.org/3/library/importlib.html#importlib.util.cache_from_source . On Python 2, you will need to reimplement that function yourself.

Ok i'll try this. It starts to be much bigger than the original change :)
By the way, I'm not used to write code for Python2/Python3
Is there a good way to do that ? What I see on forums is something like that:

if hasattr(importlib, "invalidate_caches"): importlib.invalidate_caches()

Is that right ?

Yes, though we have a distributed.compatibility module where you could add a invalidate_import_caches function.

I made the changes to delete the pyc file
It works fine now without the sleep(1) call

mrocklin · 2017-02-20T23:52:57Z

distributed/tests/test_client.py

+                os.remove('myfile.zip')
+
+    sleep(1)  # TODO:  why is this necessary?
+    x = c.submit(g, pure=False)


You probably don't need pure=False here.

In the same way pure=False is used in the other upload_file test but i agree that it could be removed.
Should I remove it from the other tests ?

mrocklin · 2017-02-20T23:53:51Z

distributed/tests/test_client.py

+        return myfile.f()
+
+    with tmp_text('myfile.py', 'def f():\n    return 123') as fn_my_file, \
+         tmp_text('init.py', '') as fn_init:


Why the file init.py?

Actually you're right. I thought that the init.py was necessary to make a valid module but actually it's not. I tried to remove it and the test passed as well.
Thx for the reveiw

pitrou · 2017-02-21T08:14:47Z

distributed/tests/test_client.py

@@ -1116,6 +1117,27 @@ def g():
    result = yield y._result()
    assert result == 456

+@gen_cluster(client=True)
+def test_upload_file_zip(c, s, a, b):


The test would be more robust if it scrubbed out myfile from sys.modules at the start and at the end.

I'm not sure to get this one.
This module is uploaded locally on workers , right ? The workers aren't restarted before each test ?

With gen_cluster, the workers should run in the current process, so they would inherit the current (process-wide) import state. For example, perhaps the myfile module seen in this test actually comes from another one of the tests here.

It seems all tests for upload_file are written in this style. I think it would deserve fixing.

ok, so i'm going to apply all those modifications to all tests :)

all the upload_file_* tests i mean

pitrou · 2017-02-21T08:15:18Z

distributed/worker.py

+                    logger.info("Reload module %s from .zip file", name)
+                    if out_filename not in sys.path:
+                        sys.path.insert(0, out_filename)
+                    name = name.split('-')[0]


Just a question: why this line?

I kept this line from ('.py', '.pyc') case

if ext in ('.py', '.pyc'): logger.info("Reload module %s from .py file", name) name = name.split('-')[0] reload(import_module(name))

Actually I don't know what it is for, so i can remove it in the zip case

pitrou · 2017-02-21T08:16:38Z

distributed/worker.py

+                    if out_filename not in sys.path:
+                        sys.path.insert(0, out_filename)
+                    name = name.split('-')[0]
+                    reload(import_module(name))


invalidate_caches should probably be called just before this line.

bmaisonn · 2017-02-21T14:57:01Z

It starts to be confusing here...
I modified my test to integrate your reviews (Using invalid_cache, remove sleep and pure=False).
I also modified the test to make the upload twice with different code and check if the modifications are correctly taken into account (As it's done in test_upload_file):

@gen_cluster(client=True)
def test_upload_file_zip(c, s, a, b):
    def g():
        import myfile
        return myfile.f()

    try:
        for value in [123, 456]:
            with tmp_text('myfile.py', 'def f():\n    return {}'.format(value)) as fn_my_file:
                with zipfile.ZipFile('myfile.zip', 'w') as z:
                    z.write(fn_my_file, arcname=os.path.basename(fn_my_file))
                yield c._upload_file('myfile.zip')
                    
                x = c.submit(g)
                result = yield x._result()
                assert result == value
    finally:
        if os.path.exists('myfile.zip'):
            os.remove('myfile.zip')
        if 'myfile' in sys.modules:
            del sys.modules['myfile']
        for path in sys.path:
            if os.path.basename(path) == 'myfile.zip':
                sys.path.remove(path)
                break

With this code the second test assert result == 456 fails as result == 123. So the second file isn't updated.
The same happens in test_upload_file is I remove pure=False OR sleep(1).
I really don't understand why it fails here ...

mrocklin · 2017-02-21T15:06:16Z

You need the pure=False if you're doing this twice and expect to get different results. Otherwise the second call will just reference the first.

…

On Tue, Feb 21, 2017 at 9:57 AM, bmaisonn ***@***.***> wrote: It starts to be confusing here... I modified my test to integrate your reviews (Using invalid_cache, remove sleep and pure=False). I also modified the test to make the upload twice with different code and check if the modifications are correctly taken into account (As it's done in test_upload_file): @gen_cluster(client=True) def test_upload_file_zip(c, s, a, b): def g(): import myfile return myfile.f() try: for value in [123, 456]: with tmp_text('myfile.py', 'def f():\n return {}'.format(value)) as fn_my_file: with zipfile.ZipFile('myfile.zip', 'w') as z: z.write(fn_my_file, arcname=os.path.basename(fn_my_file)) yield c._upload_file('myfile.zip') x = c.submit(g) result = yield x._result() assert result == value finally: if os.path.exists('myfile.zip'): os.remove('myfile.zip') if 'myfile' in sys.modules: del sys.modules['myfile'] for path in sys.path: if os.path.basename(path) == 'myfile.zip': sys.path.remove(path) break With this code the second test assert result == 456 fails as result == 123. So the second file isn't updated. The same happens in test_upload_file is I remove pure=False OR sleep(1). I really don't understand why it fails here ... — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#886 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AASszLL-ff6wNyoy1qa8mBQqItNkB5Qfks5revs9gaJpZM4MGr5t> .

bmaisonn · 2017-02-21T15:31:57Z

Ok, i misunderstood the meaning of pure.
It works now
Thx

…e now remove the pyc files corresponding to the .py file being uploaded As importlib.invalidate_caches and importlib.util.cache_from_source are python3 specifc move them in compatibility (+1 squashed commits) Squashed commits: [92d94b4] Code reviews: In worker.py::upload_file: Commonalize code, Remove split('-') as it seems useless, Use invalid_caches before reloading modules In test_client.py::test_upload_file_*: commonalize code Make two tries to check for upload updates

bmaisonn · 2017-02-22T10:31:28Z

I commited a new change to integrate the reviews
Both tests pass in python 2 and python 3

pitrou · 2017-02-22T13:03:44Z

distributed/worker.py

+                else:
+                    for name in names_to_import:
+                        logger.info("Reload module %s from %s file", name, ext)
+                        invalidate_caches()


Just a nit: you can pull invalidate_caches() out of the for loop.

pitrou · 2017-02-22T13:05:11Z

Thanks a lot!
There's one thing missing: you should update the Client.upload_file docstring.

bmaisonn · 2017-02-22T13:18:43Z

I moved the call to invalidate_caches and updated the documentation
Thx for you help!

pitrou · 2017-02-22T13:38:08Z

I don't have anything to add here, perhaps @mrocklin wants to take a last look.

mrocklin · 2017-02-22T14:01:58Z

Happy to trust you two on this. Thank you both for the effort and review.

Support to upload modules from zip files as detailled in ZipImport do…

74e72b0

…cumentation

mrocklin reviewed Feb 20, 2017

View reviewed changes

pitrou reviewed Feb 21, 2017

View reviewed changes

pitrou reviewed Feb 22, 2017

View reviewed changes

Bertrand Maisonneuve added 2 commits February 22, 2017 14:08

Update update_client document

13b35d5

Move call to invalidate_caches out of the for loop

fd9c21d

pitrou merged commit 050c2f9 into dask:master Feb 22, 2017

pitrou mentioned this pull request Feb 28, 2017

Uploading zip module #865

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support to upload modules from zip files #886

Support to upload modules from zip files #886

bmaisonn commented Feb 20, 2017

mrocklin left a comment

mrocklin Feb 20, 2017

pitrou Feb 21, 2017

bmaisonn Feb 21, 2017

pitrou Feb 21, 2017

bmaisonn Feb 21, 2017

bmaisonn Feb 21, 2017

pitrou Feb 21, 2017

bmaisonn Feb 21, 2017

pitrou Feb 21, 2017

bmaisonn Feb 22, 2017

mrocklin Feb 20, 2017

bmaisonn Feb 21, 2017

mrocklin Feb 20, 2017

bmaisonn Feb 21, 2017

pitrou Feb 21, 2017

bmaisonn Feb 21, 2017

pitrou Feb 21, 2017

bmaisonn Feb 21, 2017

bmaisonn Feb 21, 2017

pitrou Feb 21, 2017

bmaisonn Feb 21, 2017

pitrou Feb 21, 2017

bmaisonn commented Feb 21, 2017

mrocklin commented Feb 21, 2017 via email

bmaisonn commented Feb 21, 2017

bmaisonn commented Feb 22, 2017

pitrou Feb 22, 2017

pitrou commented Feb 22, 2017

bmaisonn commented Feb 22, 2017

pitrou commented Feb 22, 2017

mrocklin commented Feb 22, 2017

Support to upload modules from zip files #886

Support to upload modules from zip files #886

Conversation

bmaisonn commented Feb 20, 2017

mrocklin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmaisonn commented Feb 21, 2017

mrocklin commented Feb 21, 2017 via email

bmaisonn commented Feb 21, 2017

bmaisonn commented Feb 22, 2017

Choose a reason for hiding this comment

pitrou commented Feb 22, 2017

bmaisonn commented Feb 22, 2017

pitrou commented Feb 22, 2017

mrocklin commented Feb 22, 2017