Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add python3 support #196

Merged
merged 19 commits into from Feb 15, 2017
Merged

Add python3 support #196

merged 19 commits into from Feb 15, 2017

Conversation

jcfr
Copy link
Collaborator

@jcfr jcfr commented Feb 14, 2017

This set of changes supersedes #195.

It includes changes required to run pyradiomics on Python 3. To gracefully handles the different between py2 and py3, it introduces the dependency on six. See https://pythonhosted.org/six/

Notes:

  • instead of switching to using dict.items() (which is less efficient in py2), it uses six.iteritems
  • since MacOSX SimpleITK python wheels are not (yet) available for Python 3.x, corresponding TravisCI builds haven't been enabled.

jcfr and others added 16 commits February 14, 2017 10:54
…2 and py3

Six provides simple utilities for wrapping over differences between
Python 2 and Python 3. It is intended to support codebases that work
on both Python 2 and 3 without modification. six consists of only one
Python file, so it is painless to copy into a project.

See https://pythonhosted.org/six/
six.string_types represents the possible types for text data. This
is basestring() in Python 2 and str in Python 3.
This was done with the help of 2to3 refactoring tool and the addition
of "from __future__ import print_function" where appropriate:

$ 2to3 -f print .     # dry-run
$ 2to3 -f print -w .  # proceed to changes
From six documentation [1]:

  Get the next item of iterator it. StopIteration is raised if
  the iterator is exhausted. This is a replacement for calling
  it.next() in Python 2 and next(it) in Python 3.

[1] https://pythonhosted.org/six/#object-model-compatibility
From six documentation [1]:

  Returns an iterator over dictionary‘s items. This replaces
  dictionary.iteritems() on Python 2 and dictionary.items() on
  Python 3. kwargs are passed through to the underlying method.

[1] https://pythonhosted.org/six/#object-model-compatibility
To facilitate the update of the notebooks, a convenient script named
"fix_notebook_print.py" has been used.

Then, import of print_function was manually added.

$ ack "print " -l
bin/Notebooks/helloRadiomics.ipynb
bin/Notebooks/helloFeatureClass.ipynb
bin/Notebooks/PyRadiomics Example.ipynb

$ /tmp/fix_ipython_notebook_print.py ./bin/Notebooks/helloRadiomics.ipynb bin/Notebooks/helloFeatureClass.ipynb bin/Notebooks/PyRadiomics\ Example.ipynb
Fixing ./bin/Notebooks/helloRadiomics.ipynb - done (9 lines updated)
Fixing bin/Notebooks/helloFeatureClass.ipynb - done (37 lines updated)
Fixing bin/Notebooks/PyRadiomics Example.ipynb - done (25 lines updated)

Script available here: https://gist.github.com/jcfr/7366dda36f86deaea2d81ffee160bf3f
Update of the code was facilitated by the use of the 2to3 refactoring tool:

$ 2to3 -f dict .      # dry-un
$ 2to3 -f dict -w .   # recactor

Updates where code was (1) iterating of the keys values or (2) getting the number
of keys wasn't updates.

Co-authored-by: Joost van Griethuysen <j.v.griethuysen@nki.nl>
This commit fixes the following error reported when running the tests
using python3:

  File "/home/jcfr/Projects/pyradiomics/tests/testUtils.py", line 184, in readBaselineFiles
    headers = six.next(csvReader)
  _csv.Error: iterator should return strings, not bytes (did you open the file in text mode?)
With newer versions of SimpleITK and python, a tuple or list is expected.
Direcly passing a numpy array is not supported.

This commit fixes error like the following:

  File "/home/jcfr/Projects/pyradiomics/radiomics/imageoperations.py", line 125, in cropToTumorMask
    cif.SetLowerBoundaryCropSize(ijkMinBounds)
  File "/home/jcfr/.virtualenvs/pyradiomics-py3/lib/python3.5/site-packages/SimpleITK/SimpleITK.py", line 21397, in SetLowerBoundaryCropSize
    return _SimpleITK.CropImageFilter_SetLowerBoundaryCropSize(self, LowerBoundaryCropSize)
  TypeError: in method 'CropImageFilter_SetLowerBoundaryCropSize', argument 2 of type 'std::vector< unsigned int,std::allocator< unsigned int > > const &'

Co-authored-by: Jean-Christophe Fillion-Robin <jchris.fillionr@kitware.com>
In python 3.5 GLRLM results differed from baseline. This was due to
the different implementation of the `filter()` function, which returns
an iterator instead of a list. In GLRLM calculation, there where 2
iterations over this iter-object (one for flat-angle check, one for
matrix calculation), causing it to break in python 3.5.

Fix by merging the 2 iterations into one loop.

Additionally, simplify the filter function.
…str"

Some functions expect string types (such as in nose-testing), but
python 3.5 by default provides unicode strings. This commit casts
where necessary.

Also change csv.writer mode from 'wb' to 'w', which accepts regular strings.
Since in Python3, zip() statement returns an iterator, this commit use
the list() statement to exhaust the iterator and initialize the numpy
array.

Co-authored-by: Jean-Christophe Fillion-Robin <jchris.fillionr@kitware.com>
Since no MacOSX SimpleITK wheels for Python 3.x is available, corresponding
Travis builds havn't been enabled.
# - MANYLINUX_IMAGE=manylinux-x64 MANYLINUX_PYTHON=cp35-cp35m
# - MANYLINUX_IMAGE=manylinux-x86 MANYLINUX_PYTHON=cp35-cp35m
- MANYLINUX_IMAGE=manylinux-x64 MANYLINUX_PYTHON=cp33-cp33m
- MANYLINUX_IMAGE=manylinux-x86 MANYLINUX_PYTHON=cp33-cp33m
Copy link
Collaborator

Choose a reason for hiding this comment

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

These two lines cause CircleCI to fail, as it tries to install numpy == 1.12.0, which does not support python 3.3 anymore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, I am now looking into adding few env. markers

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thinking more about it, as you suggested let's just remove support for Python 3.3. It is now worth the complication

If really needed, we could look into specifying some more complex
requirements using Environment Markers [1]

[1] https://www.python.org/dev/peps/pep-0496/

This commit avoids to get the following error:

Collecting numpy>=1.9.2 (from -r requirements.txt (line 1))
  Downloading numpy-1.12.0.zip (4.8MB)
    100% |████████████████████████████████| 4.8MB 240kB/s
    Complete output from command python setup.py egg_info:
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/tmp/pip-build-7w8sbj/numpy/setup.py", line 34, in <module>
        raise RuntimeError("Python version 2.7 or >= 3.4 required.")
    RuntimeError: Python version 2.7 or >= 3.4 required.

Reported-by: Joost van Griethuysen <j.v.griethuysen@nki.nl>
@jcfr jcfr dismissed JoostJM’s stale review February 14, 2017 16:27

A new commit addressing the concern has been pushed

Copy link
Collaborator

@JoostJM JoostJM left a comment

Choose a reason for hiding this comment

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

Additionally, the variable in_py3 can be removed from __init__.py, as this is replaced by six.PY3

@@ -19,11 +19,6 @@ matrix:
# env:
# - PYTHON_VERSION=3.4.5
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this mean that python 3.x testing for Mac is still disabled?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

python 3.x testing for Mac is still disabled?

Indeed. Waiting for SimpleITK wheels, we do not have a choice. See https://itk.org/SimpleITKDoxygen/html/PyDownloadPage.html

Copy link

Choose a reason for hiding this comment

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

There are however conda packages for SimpleITK, would it make sense to get the mac tests working on miniconda/anaconda (https://anaconda.org/simpleitk/simpleitk) instead of standard python?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jcfr ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would make sense, the idea would be to submit a recipe to conda-forge. With that in place, the package would be available on conda on linux, macosx and windows.

That said, conda-forge recipe are updated only when there is a release of the package. If you would like to setup continuous integration using miniconda, you would have to set it up on your own.

I currently do not have the bandwidth to set miniconda CI up for pyradiomics, in the mean time I would be happy to review PR.

@blowekamp Do we know when macosx wheels will be available for python 35 ?

Choose a reason for hiding this comment

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

I was just working on some scripts for the 1.0 release... I'll see if I can run them on the release branch too like the recent x86_64 0.10.0.post3 wheel.

Suggested-by: Joost van Griethuysen <j.v.griethuysen@nki.nl>
@JoostJM
Copy link
Collaborator

JoostJM commented Feb 14, 2017

@jcfr, last build of CircleCI failed, python 3.x couldn't find venv in some commandline argument, I suspect this has something to do with the virtual environment. Can you look into it?

@jcfr
Copy link
Collaborator Author

jcfr commented Feb 14, 2017

python 3.x couldn't find venv in some commandline argument, I suspect this has something to do with the virtual environment. Can you look into it?

I noticed. I will be looking at this later today.

@jcfr
Copy link
Collaborator Author

jcfr commented Feb 14, 2017

last build of CircleCI failed, python 3.x couldn't find venv in some commandline argument, I suspect
this has something to do with the virtual environment. Can you look into it?

I identified the problem and submitted a fix to upstream setuptools. See pypa/setuptools#971

In the mean time, I will add a workaround.

…ptools issue

Since:
 (1) CircleCI automatically adds a symlink from /home/ubuntu/pyradiomics/venv
     to /home/ubuntu/virtualenvs/venv-system, (indeed it detects it is a
     python project)

 (2) the symlink is indeed invalid when "seen" from within the docker
     container building the project (because it mount the current folder
     into the dockcross/manylinux-x64 image),

 (3) distutils available in python 3.4.6 has an issue causing sdist to fail
     when it find an invalid symlink (http://bugs.python.org/issue12885)
     Setuptools is also failing to monkey patch for that version of python.
     A patch has been submitted upstream: pypa/setuptools#971

To workaround the issue, we simply remove the symbolic link and avoid the
problem completely.
@jcfr
Copy link
Collaborator Author

jcfr commented Feb 14, 2017

Et voila. As soon as, Travis returns green. Will be integrating 🎆

@jcfr jcfr dismissed JoostJM’s stale review February 14, 2017 23:48

All comments have been addressed. CircleCI is now green :)

@jcfr jcfr merged commit 84dc727 into master Feb 15, 2017
@jcfr jcfr deleted the add-python3-support branch February 15, 2017 01:35
@JoostJM JoostJM added this to Python 2/3 Compatability in PyRadiomics build/installation Feb 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
PyRadiomics build/installation
  
32/64 bits Python 2/3 Compatability
Development

Successfully merging this pull request may close these issues.

None yet

4 participants