Skip to content

Move Array creation to its own module and out of Context#252

Closed
cowlicks wants to merge 10 commits into
enthought:masterfrom
cowlicks:move-array-creation
Closed

Move Array creation to its own module and out of Context#252
cowlicks wants to merge 10 commits into
enthought:masterfrom
cowlicks:move-array-creation

Conversation

@cowlicks
Copy link
Copy Markdown
Contributor

The array creation routines fromfunction, zeros, ones, empty, and fromndarray/fromarray are now in their own module, and not in the Context class. They use the distarray.world.WORLD context by default, this can be changed by passing them the context= kwarg.

@kwmsmith
Copy link
Copy Markdown
Contributor

It looks like both Py2 and Py3 hang on test_barrier (distarray.tests.test_decorators.TestLocalDecorator) -- can you look into it?

@cowlicks cowlicks changed the title Move Array creation to its module and out of Context Move Array creation to its own module and out of Context Mar 25, 2014
@cowlicks
Copy link
Copy Markdown
Contributor Author

This is pretty weird. I think it might be an issue with multiple COMM.WORLDs being created? Still investigating.

@cowlicks
Copy link
Copy Markdown
Contributor Author

The hanging was due to a circular import. I fixed this by lazily importing the offending module. I think this is G2G.

Closes issue #35 and possibly #192.

@kwmsmith
Copy link
Copy Markdown
Contributor

@cowlicks please look into the Travis failures.

@cowlicks
Copy link
Copy Markdown
Contributor Author

I'm down to one error now. Any ideas? @kwmsmith

@kwmsmith
Copy link
Copy Markdown
Contributor

I've seen errors like this before -- will take a look once I have some bandwidth...

@kwmsmith kwmsmith added this to the 0.2 milestone Mar 29, 2014
@kwmsmith
Copy link
Copy Markdown
Contributor

I've tracked down this bug -- it's the same IndexError that @markkness is seeing with from_dim_data().

Will put it in a separate issue.

@kwmsmith
Copy link
Copy Markdown
Contributor

@cowlicks now that #259 is fixed and #263 is merged in master, please update and retest to see if that fixes the issue you're having.

@cowlicks
Copy link
Copy Markdown
Contributor Author

@kwmsmith done

@bgrant
Copy link
Copy Markdown
Contributor

bgrant commented Mar 31, 2014

It's kind of weird to have from_dim_data, fromfunction, fromndarray etc. not attached to a class. That naming scheme makes me think "classmethod". Maybe for a future PR, but how hard would it be to make these classmethods on DistArray instead? Or, are there better names?

Also, should we put _s in all of them, or rename from_dim_data to fromdimdata?

@cowlicks
Copy link
Copy Markdown
Contributor Author

I like the idea of naming consistency. These would do the same thing as class methods except it would be awkward to write DistArray.fromarray(arr) every time you wanted to do that. At least it would seem awkward from someone coming from NumPy.

@kwmsmith
Copy link
Copy Markdown
Contributor

We could have a fully consistent core with from_function, from_ndarray, and from_dim_data all classmethods on DistArray, and have a NumPy compatibility layer on top of everything to match NumPy's API as closely as possible.

That would provide an easier path for people to get things working with existing NumPy code, and then gradually shift over to the core API when they want to use DistArray specific features.

@bgrant
Copy link
Copy Markdown
Contributor

bgrant commented Mar 31, 2014

The test suite seems to be hanging on my local machine. With Python3, it hangs right after

Test that the same seed generates the same sequence. ... ok

With Python2, it hangs after a umath test...

details forthcoming.

I restarted the tests on Travis just to see if they pass again.

@bgrant
Copy link
Copy Markdown
Contributor

bgrant commented Mar 31, 2014

Now Python2 is hanging while running and HDF5 test...

test_save_3d (distarray.tests.test_distributed_io.TestHdf5FileSave) ...

@bgrant
Copy link
Copy Markdown
Contributor

bgrant commented Mar 31, 2014

Travis seems to be passing. Anyone else seeing hangs locally on OS X? I'm having trouble reproducing it now.

@kwmsmith
Copy link
Copy Markdown
Contributor

kwmsmith commented Apr 1, 2014

No hangs for me.

@kwmsmith
Copy link
Copy Markdown
Contributor

kwmsmith commented Apr 1, 2014

Closes #192

Comment thread distarray/creation.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you go into detail why it leads to a circular import? What are the circular dependencies? I'd like to see if we can resolve the circular dependency rather than using a decorator.

While this decorator solves the problem, it does it in a way that strikes me as too implicit. I think something like this would be more explicit; am curious what your thoughts are:

def zeros(..., context=None, ...):
    context = context or WORLD
    ...

It's one extra line per function, but it's fairly trivial, as long as we can resolve the circular dependency issue. And if we can't, then it's just two lines -- one for the import, and one for the assignment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the steps are:

  1. import distarray
  2. in __init__.py we do from distarray.creation import *
  3. in there we do from distarray.world import WORLD.
  4. which does from distarray.context import Context
  5. which does import distarray (goto 1.)

But I don't think client.py needs to do import distarray it could probably do import distarray.functions as ... instead. I'll try it out.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This python -vv output is a little more helpful:

import distarray # directory distarray
# distarray/__init__.pyc matches distarray/__init__.py
import distarray # precompiled from distarray/__init__.pyc
# distarray/creation.pyc matches distarray/creation.py
import distarray.creation # precompiled from distarray/creation.pyc
# distarray/world.pyc matches distarray/world.py
import distarray.world # precompiled from distarray/world.pyc
Exception AttributeError: "'Context' object has no attribute 'key_context'" in <bound method Context.__del__ of <distarray.context.Context object at 0x109ae9f90>> ignored
import distarray # directory distarray
# distarray/__init__.pyc matches distarray/__init__.py
import distarray # precompiled from distarray/__init__.pyc
# distarray/creation.pyc matches distarray/creation.py
import distarray.creation # precompiled from distarray/creation.pyc
# distarray/world.pyc matches distarray/world.py
import distarray.world # precompiled from distarray/world.pyc
Exception AttributeError: "'Context' object has no attribute 'key_context'" in <bound method Context.__del__ of <distarray.context.Context object at 0x10afd9890>> ignored
import distarray # directory distarray
# distarray/__init__.pyc matches distarray/__init__.py
import distarray # precompiled from distarray/__init__.pyc
# distarray/creation.pyc matches distarray/creation.py
import distarray.creation # precompiled from distarray/creation.pyc
# distarray/world.pyc matches distarray/world.py
import distarray.world # precompiled from distarray/world.pyc
Exception AttributeError: "'Context' object has no attribute 'key_context'" in <bound method Context.__del__ of <distarray.context.Context object at 0x10afd9890>> ignored

@cowlicks cowlicks modified the milestone: 0.2 Apr 2, 2014
@cowlicks
Copy link
Copy Markdown
Contributor Author

cowlicks commented Apr 2, 2014

Hangs on OSX seem to be fixed by upgrading to the pyzmq which is available on pypi (14.1.1). It is unclear what version of pyzmq is required but hangs were seen with v13.1.0

@cowlicks cowlicks mentioned this pull request Apr 3, 2014
@cowlicks
Copy link
Copy Markdown
Contributor Author

cowlicks commented Apr 3, 2014

rebased

@kwmsmith
Copy link
Copy Markdown
Contributor

kwmsmith commented Apr 3, 2014

@cowlicks as I think about this PR, I'm concerned about the required upgrade to a newer version of zeromq / pyzmq. IPython is used widely, and I'd like to be able to sit on top of existing IPython installations that aren't the latest -- if we tell people that they have to upgrade their IPython (or underlying components of IPython), then that may drive many away. At the very least, I'd like to better understand what is causing the issue here, and what changed in recent versions of zmq / pyzmq to fix the issue. That could take a while.

To help resolve this, please write up a summary of the IPython-specific things you're doing in this PR, and we'll take that to the IPython guys for input and advice.

Until we can get some traction, I'm hesitant to merge this PR.

@cowlicks
Copy link
Copy Markdown
Contributor Author

cowlicks commented Apr 3, 2014

I have not been able to reproduce the reported hanging. I've tried @kwmsmith @bgrant can y'all try to install an earlier versions of pyzmq? This should do it.

$ pip uninstall pyzmq
$ pip install -Iv pyzmq==2.2

Then see if the hanging occurs again?

@kwmsmith kwmsmith modified the milestones: 0.3, 0.2 Apr 4, 2014
@kwmsmith
Copy link
Copy Markdown
Contributor

kwmsmith commented Apr 4, 2014

moving to next milestone.

@bgrant
Copy link
Copy Markdown
Contributor

bgrant commented Apr 9, 2014

I get all kinds of errors on master after installing pyzmq 2.2.

@kwmsmith
Copy link
Copy Markdown
Contributor

Based on offline conversations, I'm closing this PR for now. The current decision is to make all context usage explicit, and this PR would require a lot of implicit context usage.

We can revisit later if necessary.

@kwmsmith kwmsmith closed this Apr 15, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants