Split up client and local code. by cowlicks · Pull Request #330 · enthought/distarray

cowlicks · 2014-04-21T17:37:27Z

Previously with local/ residing in distarray/* next to all the client code the client code was imported anytime we imported local/. This PR breaks up the package so that local code and client code can be imported independently. This avoi
had imports from local. This breaks up the package so that local code and client code can be imported independently. This avoids some of the circular import issues we had. And avoids import client modules on the engines.

The package structure is now like this

distarray/
    |
    -local/
    |   -local_stuff.py
    -client/
    |   -client_stuff.py
    -general_stuff.py

I also renamed the client.py file to distarray.py.

kwmsmith · 2014-04-21T18:37:10Z

@bgrant @cowlicks thoughts on renaming client / client_stuff to global / global_stuff? It would compliment local better, and is less IP.parallel centric, preparing the way for implementing an MPI-only backend.

bgrant · 2014-04-21T19:12:39Z

Doesn't this run into reserved-word issues?

cowlicks · 2014-04-21T20:47:50Z

I'm not sure what you mean @bgrant ?

markkness · 2014-04-21T21:22:13Z

I think he means the proposed use of the name global, which is a reserved
word. I suggest not trying to use reserved words.

On Mon, Apr 21, 2014 at 3:47 PM, Blake Griffith notifications@github.comwrote:

I'm not sure what you mean @bgrant https://github.com/bgrant ?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/330#issuecomment-40975366
.

cowlicks · 2014-04-21T21:54:16Z

Oh yeah, @kwmsmith's suggestion does.

kwmsmith · 2014-04-21T21:56:47Z

I rescind my suggestion -- don't know what I was thinking...

Other options besides global for this package? world? universe? Nothing comes to mind that I really like.

We could abbreviate it to glb or glbl, but those don't seem very good, either.

cowlicks · 2014-04-21T22:26:04Z

I'd prefer client since we seem to have a master/slave architecture. And the client does "master" stuff.

cowlicks · 2014-04-21T22:27:14Z

rebased

kwmsmith · 2014-04-21T22:28:19Z

But client is definitely IPython.parallel terminology. It would be a strange name for someone coming from MPI, for example.

What about root? Or master?

cowlicks · 2014-04-22T17:01:55Z

I like client because it implies it is the interface through which we control distarray. Other suggested names don't really do this, except master, but this has git connotations. controller, interface, or manager would work.

I'd like to steer away from prepositional phrases like local, global, remote etc. since their meaning is relative to the location you are considering, and instead use the relationship between the client and engines for naming. Since this is the same universally.

cowlicks · 2014-04-22T17:02:28Z

Also, I'll hold off rebasing this until we think it is ready to merge since it will probably break again.

cowlicks · 2014-04-22T21:09:45Z

Rebased. Did we decide what to rename client? I like to get this merged soon since it touches so much stuff.

bgrant · 2014-04-24T22:13:56Z

@kwmsmith, @cowlicks: Why don't we go ahead and merge this to get the breakage out of the way. It'll be easier to do a rename when we decide on new names.

cowlicks · 2014-04-25T14:45:24Z

@bgrant okay. Changing the name will be easier once this is in. I'll rebase it and go with dist and local

kwmsmith · 2014-04-26T21:07:27Z

@bgrant @cowlicks +1 on merging for now, and resolving the final name later. Reviewing.

kwmsmith · 2014-04-29T18:47:32Z

This isn't necessary (and it's broken -- cleanup_all takes 2 arguments).

Look at the Context._CLEANUP = (...) inside Context.__init__(). That's where the cleanup stuff is registered.

cowlicks · 2014-04-29T20:46:44Z

Rebased and working

kwmsmith · 2014-04-30T16:40:20Z

@cowlicks -- looks good. Some small comments:

It seems like mpiutils.py should go inside local.
Similarly, ipython_utils.py should go inside dist.
cleanup.py should go inside dist, I think.
I'd prefer if distarray/utils.py were split into distarray/local/utils.py and distarray/dist/utils.py, but that isn't so important right now, I think.

Ideally we'd have just the local, dist, externals, plotting, apps and tests packages inside distarray, but we don't have to do everything in this PR.

cowlicks · 2014-04-30T19:58:19Z

@kwmsmith mpiutils is used in context.py so it does not make sense for it to be in local/. What if all the the *utils.py modules go in distarray/utils/*?

kwmsmith · 2014-04-30T20:06:23Z

On Wed, Apr 30, 2014 at 2:58 PM, Blake Griffith notifications@github.comwrote:

mpiutils is used in context.py so it does not make sense for it to be in
local/.

It's used inside functions that are pushed locally with apply_async, so
it is only ever used on the engines. It is never imported directly by
anything inside context.py

What if all the the utils.py modules go in distarray/utils/?

Only for those that really need to be used both locally and globally.
mpiutils should only ever need to be used locally.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/330#issuecomment-41842795
.

Kurt W. Smith, Ph.D. ksmith@enthought.com
Enthought, Inc. http://www.enthought.com | 512.536.1057

cowlicks · 2014-05-01T14:07:50Z

Rebased

bgrant · 2014-05-01T22:31:50Z

I think most of the tests in distarray/tests should go in distarray/dist/tests. I like to have code structured like the following:

my_module.py
tests/
    test_my_module.py
subpackage/
    my_other_module.py
    tests/
        test_my_other_module.py

bgrant · 2014-05-01T22:33:49Z

... so some of the tests should also move from distarray/tests to distarray/utils/tests as well.

Otherwise, looks good to me.

kwmsmith · 2014-05-01T22:43:46Z

@bgrant +1 -- @cowlicks please move whatever test files from distarray/tests to distarray/dist/tests as makes sense.

This is because dacluster should not have to impor distarray/client/ to clean things up.

Since the module distarray/client/distarray.py's name conflicts with the package name we import `absolute_import` everywhere in the file.

kwmsmith · 2014-05-02T17:46:31Z

@cowlicks -- what about moving mpiutils.py to local, ipython_utils.py to dist, and cleanup.py to dist?

cowlicks · 2014-05-02T18:17:30Z

@kwmsmith oops, I thought those were already in this PR for some reason. Done.

bgrant · 2014-05-02T18:27:52Z

@cowlicks: what I was getting at here is that each module in directory <dir> should have its tests in <dir>/tests. So since metadata_utils.py and testing.py are still in distarray/, their tests should still be in distarray/tests. The modules that you moved into distarray/dist should have their tests is distarray/dist/tests/.

kwmsmith · 2014-05-02T19:08:28Z

@cowlicks looks like the dacluster script wasn't updated with the recent moves (travis fails for both 2.7 and 3.3).

Also, please organize the test files according to @bgrant's suggestion.

cowlicks · 2014-05-02T21:08:03Z

Okay trying again.

bgrant · 2014-05-05T16:20:27Z

Looks good to me.

kwmsmith added the enhancement label Apr 21, 2014

kwmsmith added this to the 0.3 milestone Apr 21, 2014

kwmsmith modified the milestone: 0.3 Apr 21, 2014

kwmsmith reviewed Apr 29, 2014
View reviewed changes

cowlicks added 2 commits May 1, 2014 19:34

Put client side modules in distarray/client/ and change imports.

4b8cfa5

move client.py -> distarray.py and change imports to reflect this.

c2db971

cowlicks added 7 commits May 1, 2014 19:36

move DISTARRAY_BASE_NAME into top level __init__.py

421d07d

This is because dacluster should not have to impor distarray/client/ to clean things up.

Fix circular import issue with distarray.py

11bd41e

client_maps.py -> maps.py. add __future__.absolute_import where needed.

047f9e3

Since the module distarray/client/distarray.py's name conflicts with the package name we import `absolute_import` everywhere in the file.

rename client directory to dist

0fade10

Remove cruft from bad rebase.

a082a31

Move client tests to dist/tests/.

4455a07

Fix a missed instance of client_map -> maps

dcd859b

Moves suggested by @kwmsmith

4c064e6

More moves and rebase fixes

767e989

bgrant merged commit 767e989 into enthought:master May 5, 2014

cowlicks deleted the split-global-local branch May 27, 2014 17:04

Conversation

cowlicks commented Apr 21, 2014

Uh oh!

kwmsmith commented Apr 21, 2014

Uh oh!

bgrant commented Apr 21, 2014

Uh oh!

cowlicks commented Apr 21, 2014

Uh oh!

markkness commented Apr 21, 2014

Uh oh!

cowlicks commented Apr 21, 2014

Uh oh!

kwmsmith commented Apr 21, 2014

Uh oh!

cowlicks commented Apr 21, 2014

Uh oh!

cowlicks commented Apr 21, 2014

Uh oh!

kwmsmith commented Apr 21, 2014

Uh oh!

cowlicks commented Apr 22, 2014

Uh oh!

cowlicks commented Apr 22, 2014

Uh oh!

cowlicks commented Apr 22, 2014

Uh oh!

bgrant commented Apr 24, 2014

Uh oh!

cowlicks commented Apr 25, 2014

Uh oh!

kwmsmith commented Apr 26, 2014

Uh oh!

kwmsmith Apr 29, 2014

Choose a reason for hiding this comment

Uh oh!

cowlicks commented Apr 29, 2014

Uh oh!

kwmsmith commented Apr 30, 2014

Uh oh!

cowlicks commented Apr 30, 2014

Uh oh!

kwmsmith commented Apr 30, 2014

Uh oh!

cowlicks commented May 1, 2014

Uh oh!

bgrant commented May 1, 2014

Uh oh!

bgrant commented May 1, 2014

Uh oh!

kwmsmith commented May 1, 2014

Uh oh!

kwmsmith commented May 2, 2014

Uh oh!

cowlicks commented May 2, 2014

Uh oh!

bgrant commented May 2, 2014

Uh oh!

kwmsmith commented May 2, 2014

Uh oh!

cowlicks commented May 2, 2014

Uh oh!

bgrant commented May 5, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants