networkx / networkx-metis

NetworkX Addon to allow graph partitioning with METIS
Other
79 stars 21 forks source link

Fix import style #10

Closed OrkoHunter closed 9 years ago

OrkoHunter commented 9 years ago

With this change and updated #3, networkx-metis works fine on python 2. But has errors with python 3.

Python 3.4.0 (default, Apr 11 2014, 13:05:11) 
Type "copyright", "credits" or "license" for more information.

In [1]: import metis
---------------------------------------------------------------------------
ImportError: Traceback (most recent call last)
<ipython-input-1-f3c558d985ea> in <module>()
----> 1 import metis

/metis.py in <module>()
     16 
     17 from decorators import convert_exceptions
---> 18 from _types import MetisError, MetisOptions
     19 from _metis import node_nd, part_graph, compute_vertex_separator
     20 __all__ = ['node_nested_dissection', 'partition', 'vertex_separator',

/_types.py in <module>()
      2 
      3 from enums import *
----> 4 from _metis import set_default_options
      5 
      6 __all__ = ['MetisOptions', 'MetisError']

ImportError: _metis.so: undefined symbol: _Py_ZeroStruct
OrkoHunter commented 9 years ago

Regarding ImportError: _metis.so: undefined symbol: _Py_ZeroStruct, after some research I came to know that _Py_ZeroStruct symbol is not defined over Python 3. The evidence of this result can also be verified with a grep in /usr/bin/ which gives positive result for the binary file python2.7 but no result for other versions of python.

OrkoHunter commented 9 years ago

Updated. we should consider using import modules; module.item() instead of from module import item

OrkoHunter commented 9 years ago

Updated _metis.pyx with

from networkx.addons.metis import _types

from networkx.addons.metis cimport _api

EDIT: It won't build at first place because cython would be able not find _api at networkx.addons.metis to cimport.

OrkoHunter commented 9 years ago

This diff works for StringIO instance has no attribute 'fileno' Now the tests are passing.

diff --git a/_metis.pyx b/_metis.pyx
index e13d742..b956af3 100644
--- a/_metis.pyx
+++ b/_metis.pyx
@@ -264,7 +264,7 @@ def part_graph(xadj, adjncy, nparts, vwgt=None, vsize=None, adjwgt=None,
         _recursive = bool(recursive)

         with tempfile.TemporaryFile() as tmp:
-            with redirect(sys.stdout, tmp), nogil:
+            with redirect(sys.__stdout__, tmp), nogil:
                 if _recursive:
                     result = _api.METIS_PartGraphRecursive(
                         &nvtxs, &ncon, _xadj, _adjncy, _vwgt, _vsize, _adjwgt,
@@ -274,7 +274,7 @@ def part_graph(xadj, adjncy, nparts, vwgt=None, vsize=None, adjwgt=None,
                         &nvtxs, &ncon, _xadj, _adjncy, _vwgt, _vsize, _adjwgt,
                         &_nparts, _tpwgts, _ubvec, _options, &objval, _part)
             tmp.seek(0)
-            msg = unicode(tmp.read(), sys.stdout.encoding)
+            msg = unicode(tmp.read(), sys.__stdout__.encoding)

         check_result(result, msg)

@@ -354,11 +354,11 @@ def node_nd(xadj, adjncy, vwgt=None, options=None):
         _iperm = <_api.idx_t*> checked_malloc(sizeof(_api.idx_t) * nvtxs)

         with tempfile.TemporaryFile() as tmp:
-            with redirect(sys.stdout, tmp), nogil:
+            with redirect(sys.__stdout__, tmp), nogil:
                 result = _api.METIS_NodeND(&nvtxs, _xadj, _adjncy, _vwgt, _options,
                                       _perm, _iperm)
             tmp.seek(0)
-            msg = unicode(tmp.read(), sys.stdout.encoding)
+            msg = unicode(tmp.read(), sys.__stdout__.encoding)

         check_result(result, msg)

@@ -425,11 +425,11 @@ def compute_vertex_separator(xadj, adjncy, vwgt=None, options=None):
         _part = <_api.idx_t*> checked_malloc(sizeof(_api.idx_t) * nvtxs)

         with tempfile.TemporaryFile() as tmp:
-            with redirect(sys.stdout, tmp), nogil:
+            with redirect(sys.__stdout__, tmp), nogil:
                 result = _api.METIS_ComputeVertexSeparator(
                     &nvtxs, _xadj, _adjncy, _vwgt, _options, &sepsize, _part)
             tmp.seek(0)
-            msg = unicode(tmp.read(), sys.stdout.encoding)
+            msg = unicode(tmp.read(), sys.__stdout__.encoding)

         check_result(result, msg)
chebee7i commented 9 years ago

Can you explain why you are reassigning sys.__stdout__ rather than sys.stdout? The double-underscored stdout is supposed to be a reference to the one true stdout. When people reassign sys.stdout, they revert to the double-underscored version. https://docs.python.org/2/library/sys.html#sys.__stdout__

OrkoHunter commented 9 years ago

I didn't read much about it but this was a fix for the failing test, I found on stackoverflow. I'll look for a possible explanation.

debugger22 commented 9 years ago

You might want to read this https://docs.python.org/3.3/library/sys.html#sys.__stdout__

OrkoHunter commented 9 years ago

It seems that nosetests is the culprit. They probably replace sys.stdout with a StringIO object so that it can capture the output easily. But maybe they do not interfere with sys.__stdout__. I realize very well about the similarities of the two, and both works while importing/compiling but nosetests are making the difference.

Source : http://stackoverflow.com/questions/30961173/sys-stdout-works-but-sys-stdout-does-not#comment49954775_30961173

OrkoHunter commented 9 years ago

There's more!

$ nosetests -s
....
----------------------------------------------------------------------
Ran 4 tests in 0.009s

OK

works with sys.stdout.

This plugin captures stdout during test execution. If the test fails or raises an error, the captured output will be appended to the error or failure output. It is enabled by default but can be disabled with the options -s or --nocapture.

http://nose.readthedocs.org/en/latest/plugins/capture.html

chebee7i commented 9 years ago

The other option mentioned on that SO post seems more robust to me than what is currently implemented (since it doesn't assume that stdout has an fd).

OrkoHunter commented 9 years ago

Or should we go with nosetests -s on Travis without any changes?

chebee7i commented 9 years ago

The nosetests -s still requires that other code has not replaced sys.stdout with a StringIO instance. So I don't think it is a general solution.

ysitu commented 9 years ago

The reason why we need to redirect stdout is that METIS outputs error messages to stdout. We need to remember that METIS is a C library. It knows nothing about Python's sys.stdout (or sys.__stdout__) object. Therefore, what is important is that the file descriptor of stdout in C, which should be 1 on common operating systems. So I think that it would not be too bad that we use

fd = sys.__stdout__.fileno() if sys.__stdout__ else 1

or even

fd = 1

Separately from obtaining the file descriptor of stdout, this thread brings up a very good problem of reliably flushing stdout. We need to handle that.

chebee7i commented 9 years ago

Got it. That makes the implementation a bit more specific than I thought---namely, it is specifically targeting stdout generated from C functions rather than Python programs. Makes sense.

ysitu commented 9 years ago

Separately from obtaining the file descriptor of stdout, this thread brings up a very good problem of reliably flushing stdout. We need to handle that.

BTW, we are dealing with this in Cython. So we do have (somewhat) direct access to C facilities. In this case, cimporting fflush and stdout from libc.stdio and fflush(stdout) should suffice.

OrkoHunter commented 9 years ago

I guess this 66d4f92f375a90b054e0de10467e44023afbb9cd is pretty much okay to do the job. sys.stdout.encoding also needed to be changed.

ysitu commented 9 years ago

Can we separate the stdout changes to another PR?

OrkoHunter commented 9 years ago

Sounds good to create new PR for last commit.

OrkoHunter commented 9 years ago

Will send a PR shortly with the changes related to stdout. Last commit removed from this PR.

OrkoHunter commented 9 years ago

@ysitu Thanks!