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

Transitioning from Python 2.7 to Python 3 #1118

Closed
wants to merge 30 commits into from
Closed

Conversation

jcfr
Copy link
Member

@jcfr jcfr commented Mar 16, 2019

@jcfr
Copy link
Member Author

jcfr commented Apr 2, 2019

Shorty, this topic will be updated. In the mean time, Slicer can be started with Python 3.

image

See https://discourse.slicer.org/t/updating-slicer-to-work-with-python-3/4662/4?u=jcfr

@jcfr jcfr force-pushed the python3-support branch from cda8ae1 to 44f28c3 Compare April 4, 2019 01:12
@@ -154,7 +154,7 @@ def onApply(self):
kernelSizePixel = self.getKernelSizePixel()
if shellMode == MEDIAL_SURFACE:
# both erosion and dilation will be applied, so kernel size must be half on each side
kernelSizePixel = [kernelSizePixel[0]/2, kernelSizePixel[1]/2, kernelSizePixel[2]/2]
kernelSizePixel = [int(kernelSizePixel[0],2), int(kernelSizePixel[1],2), int(kernelSizePixel[2],2)]
Copy link
Member Author

Choose a reason for hiding this comment

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

@lassoan Would be great if you could review the changes associated with the Segment editor effects.

Copy link
Member Author

Choose a reason for hiding this comment

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

The gist is that in 1/3 returns 0 in python 2 and returns 0.3333333 in python 3

@jcfr jcfr force-pushed the python3-support branch 2 times, most recently from f65a9be to e175d66 Compare April 4, 2019 10:06
@jcfr jcfr changed the title WIP: Python3 support Transitioning from Python 2.7 to Python 3 Apr 4, 2019
@jcfr
Copy link
Member Author

jcfr commented Apr 9, 2019

Error on macOS:

[100%] Relocate _sysconfigdata_m_darwin_.py and update pybuilddir.txt
dyld: lazy symbol binding failed: Symbol not found: _clock_gettime
  Referenced from: /Volumes/Dashboards/Experimental/jc/Slicer-Release-UnixMakefiles/python-build/lib/libpython3.6m.dylib
  Expected in: /usr/lib/libSystem.B.dylib

dyld: Symbol not found: _clock_gettime
  Referenced from: /Volumes/Dashboards/Experimental/jc/Slicer-Release-UnixMakefiles/python-build/lib/libpython3.6m.dylib
  Expected in: /usr/lib/libSystem.B.dylib

@jcfr
Copy link
Member Author

jcfr commented Apr 10, 2019

Error on macOS has been fixed in python-cmake-buildsystem/python-cmake-buildsystem#246

jcfr and others added 22 commits April 10, 2019 18:16
The technique originally described in [1] was associated with a list
of strings and not a single string. This commit ensures no extra spaces
are introduced.

[1] http://stackoverflow.com/questions/9942594/unicodeencodeerror-ascii-codec-cant-encode-character-u-xa0-in-position-20
This commit removes support for building Slicer against Python 2.7 and
instead use Python 3.6.

Co-authored-by: Isaiah Norton <isaiah.norton@gmail.com>
This commit removes the saferef module originally introduced
in r19809 (ENH: Add python module 'saferef' allowing to create
weak reference to method/function.)

Starting with Python 3.4, the weakref module available in python is
improved to provide the 'weakref.WeakMethod' class.

See https://docs.python.org/3.6/library/weakref.html#weakref.WeakMethod
This commit fixes error "SystemError: initialization of
CTKImageProcessingITKCorePythonQt raised unreported exception"

List of changes:

$ git shortlog 941c119c..af530cf6 --no-merges
Jean-Christophe Fillion-Robin (1):
      BUG: Python3: Fix initialization of PythonQt module
In Python3, there is only one kind of integer.

See https://www.python.org/dev/peps/pep-0237/
PEP 237 -- Unifying Long Integers and Integers

This commit was crafted following these steps:

- installing future

  Slicer_DIR=/path/to/Slicer-SuperBuild/Slicer-build
  ${Slicer_DIR}/../python-install/bin/PythonSlicer -m pip install
future

- applying the "lib2to3.fixes.fix_numliterals" and
  "lib2to3.fixes.fix_long" fixes

  for f in `find ./ -name "*.py"`; do \
    ${Slicer_DIR}/../python-install/bin/PythonSlicer \
      --launch futurize \
      -f lib2to3.fixes.fix_numliterals \
      -f lib2to3.fixes.fix_long -w $f; \
  done

- reviewing the changes and updating as needed.
See https://www.python.org/dev/peps/pep-3111/
PEP 3111 -- Simple input built-in in Python 3000

This commit was crafted following these steps:

- installing future

  Slicer_DIR=/path/to/Slicer-SuperBuild/Slicer-build
  ${Slicer_DIR}/../python-install/bin/PythonSlicer -m pip install future

- applying the "lib2to3.fixes.fix_raw_input" fix

  for f in `find ./ -name "*.py"`; do \
    ${Slicer_DIR}/../python-install/bin/PythonSlicer \
      --launch futurize -f lib2to3.fixes.fix_raw_input -w $f; \
  done
See https://www.python.org/dev/peps/pep-0328/
PEP 328 -- Imports: Multi-Line and Absolute/Relative

This commit was crafted following these steps:

- installing future

  Slicer_DIR=/path/to/Slicer-SuperBuild/Slicer-build
  ${Slicer_DIR}/../python-install/bin/PythonSlicer -m pip install
future

- applying the "libfuturize.fixes.fix_absolute_import" fix

  for f in `find ./ -name "*.py"`; do \
    ${Slicer_DIR}/../python-install/bin/PythonSlicer \
      --launch futurize -f libfuturize.fixes.fix_absolute_import -w $f; \
  done

- reverting addition of "from __future__ import absolute_import"

  for f in `find ./ -name "*.py"`; do \
    sed -i '/from __future__ import absolute_import/ d' $f; \
  done
This changes is required because import of other effects are done
relatively to the package where effects are already imported using
"from .NameEffect import *".
In Python3, the division is a "true division" and not a "floor division"

See https://www.python.org/dev/peps/pep-0238/
PEP 238 -- Changing the Division Operator

This commit was crafted applying the following steps:

- installing future

  Slicer_DIR=/path/to/Slicer-SuperBuild/Slicer-build
  ${Slicer_DIR}/../python-install/bin/PythonSlicer -m pip install
future

- applying the "libfuturize.fixes.fix_division_safe" fix

  for f in `find ./ -name *.py`; do \
    ${Slicer_DIR}/../python-install/bin/PythonSlicer \
      --launch futurize -f libfuturize.fixes.fix_division_safe -w $f; \
  done

- reverting addition of "from __future__ import division"
  and "from past.utils import old_div"

  for f in `find ./ -name "*.py"`; do \
    sed -i '/from __future__ import division/ d' $f; \
    sed -i '/from past.utils import old_div/ d' $f; \
  done

- removing use of "old_div" function and replace with use of int() where
  relevant. This is step is *NOT* automated.
This commit fixes imports associated with tkinter, http, socketserver
and urllib modules.

List of changes were identified applying the libfuturize.fixes.fix_future_standard_library
fix.
These issues were identified applying the
libfuturize.fixes.fix_future_standard_library_urllib fix.
This commit was crafted by:
- applying the "libfuturize.fixes.fix_xrange_with_import" fix
- removing addition of "from builtins import range"
- removing unneeded explicit conversion to `list`. Indexing into
  the object returned by `range()` function is possible.
In python 3, strings are in unicode.
See https://nedbatchelder.com/text/unipain/unipain.html#30

This commit was crafted by

- installing future

  Slicer_DIR=/path/to/Slicer-SuperBuild/Slicer-build
  ${Slicer_DIR}/../python-install/bin/PythonSlicer -m pip install
future

- applying the "libfuturize.fixes.fix_unicode_keep_u" fix

  for f in `find ./ -name "*.py"`; do \
    ${Slicer_DIR}/../python-install/bin/PythonSlicer \
      --launch futurize -f libfuturize.fixes.fix_unicode_keep_u -w $f; \
  done

- removing implementation of "def __unicode__(self)"

- removing use of literal u"some text".

- removing explicit conversion of string to unicode
This commit fixes the following error:

 Traceback (most recent call last):
   File "/path/to/Slicer-SuperBuild/Slicer-build/lib/Slicer-4.11/qt-scripted-modules/DICOMPatcher.py", line 663, in test_DICOMPatcher1
     logic.patchDicomDir(inputTestDir, outputTestDir)
   File "/path/to/Slicer-SuperBuild/Slicer-build/lib/Slicer-4.11/qt-scripted-modules/DICOMPatcher.py", line 564, in patchDicomDir
     patchedFilePath = rule.generateOutputFilePath(ds, patchedFilePath)
   File "/path/to/Slicer-SuperBuild/Slicer-build/lib/Slicer-4.11/qt-scripted-modules/DICOMPatcher.py", line 455, in generateOutputFilePath
     patientNameID = ds.PatientName+"*"+ds.PatientID
 TypeError: unsupported operand type(s) for +: 'PersonName3' and 'str'
List of LandmarkRegistration changes:

$ git shortlog 6569f69..7fc2acc --no-merges
Jean-Christophe Fillion-Robin (1):
      ENH: Support for Python3

Pablo Hernandez-Cerdan (1):
      BUG: Fix failing test_LandmarkRegistrationVTKv6Picking due to SampleDataSource constructor

List of MultiVolumeExplorer changes:

$ git shortlog 1618048..299f2d9 --no-merges
Jean-Christophe Fillion-Robin (1):
      ENH: Support for Python3
@jcfr jcfr force-pushed the python3-support branch from 964f7d1 to 8a3629e Compare April 10, 2019 18:16
This commit fixes the following error:

  ModuleNotFoundError: No module named '_distutils_findvs'

List of changes:

$ git shortlog b27121b..610e8256 --no-merges
Jean-Christophe Fillion-Robin (1):
      Add support for building _distutils_findvs module introduced in Python 3.6
@jcfr jcfr force-pushed the python3-support branch from b469be6 to 787aa20 Compare April 11, 2019 03:16
@jcfr
Copy link
Member Author

jcfr commented Apr 11, 2019

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Users/kitware/Desktop/Slicer.app/Contents/lib/Python/lib/python3.6/imp.py", line 170, in load_source
    module = _exec(spec, sys.modules[name])
  File "<frozen importlib._bootstrap>", line 618, in _exec
  File "<frozen importlib._bootstrap_external>", line 674, in exec_module
  File "<frozen importlib._bootstrap_external>", line 781, in get_code
  File "<frozen importlib._bootstrap_external>", line 741, in source_to_code
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/Users/kitware/Desktop/Slicer.app/Contents/lib/Slicer-4.11/qt-scripted-modules/SimpleFilters.py", line 228
    print "\n".join(printStr)
             ^
SyntaxError: invalid syntax
loadSourceAsModule - Failed to load file "/Users/kitware/Desktop/Slicer.app/Contents/lib/Slicer-4.11/qt-scripted-modules/SimpleFilters.py"  as module "SimpleFilters" !
Fail to instantiate module  "SimpleFilters"
Switch to module:  "Welcome"
Traceback (most recent call last):
  File "/Users/kitware/Desktop/Slicer.app/Contents/lib/Slicer-4.11/qt-scripted-modules/DICOM.py", line 76, in performPostModuleDiscoveryTasks
    DICOMLib.setDatabasePrecacheTags()
  File "/Users/kitware/Desktop/Slicer.app/Contents/lib/Slicer-4.11/qt-scripted-modules/DICOMLib/DICOMWidgets.py", line 41, in setDatabasePrecacheTags
    plugin = slicer.modules.dicomPlugins[pluginClass]()
  File "/Users/kitware/Desktop/Slicer.app/Contents/lib/Slicer-4.11/qt-scripted-modules/MultiVolumeImporterPlugin.py", line 57, in __init__
    for tagName,tagVal in self.multiVolumeTags.iteritems():
AttributeError: 'dict' object has no attribute 'iteritems'

jcfr added 2 commits April 11, 2019 00:12
List of changes:

$ git shortlog ce324adb..3edd1bc --no-merges
Jean-Christophe Fillion-Robin (3):
      STYLE: Fix MultiVolumeImporterPlugin.py line endings
      STYLE: Apply lib2to3.fixes.fix_idioms to support python3
      ENH: Support for Python3
List of changes:

$ git shortlog 8c27c74..8cc87a5 --no-merges
Jean-Christophe Fillion-Robin (1):
      ENH: Support for Python3
@jcfr
Copy link
Member Author

jcfr commented Apr 11, 2019

Closing. Corresponding changes have been integrated in r28100 to r28129

@jcfr jcfr closed this Apr 11, 2019
@jcfr jcfr deleted the python3-support branch April 11, 2019 20:08
@cpinter
Copy link
Member

cpinter commented Apr 11, 2019

I tried building the latest revision using VS2019 (2015 toolset), and I got this when the superbuild was configuring CTK:

LINK : fatal error LNK1104: cannot open file 'C:\d\S4D\python-install\libs\python36.lib' [C:\d\S4D\CTK-build\PythonQt-build\PythonQt.vcxproj] [C:\d\S4D\CTK-build\PythonQt.vcxproj]
Performing update step for 'QtTesting'

There is no python36.lib, but there is a python27.lib in that directory.

This was my first build with 2019, but the error is about Python3, and I heard that Slicer builds well using 2019, so thought this could be related to these latest changes.

@lassoan
Copy link
Contributor

lassoan commented Apr 12, 2019

A clean build with VS2017 (VS2015 toolset) failed for me on Windows, too:

18>ClCompile:
18>  C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\bin\x86_amd64\CL.exe /c /I"C:\D\S4R\Python-3.6.7\Include" /I"C:\D\S4R\Python-3.6.7\PC" /I"C:\D\S4R\Python-3.6.7\Python" /nologo /W2 /WX- /O2 /Ob1 /D WIN32 /D _WINDOWS /D NDEBUG /D Py_BUILD_CORE /D "CMAKE_INTDIR=\"Release\"" /D _MBCS /Gm- /MD /GS /fp:precise /Zc:wchar_t /Zc:forScope /Zc:inline /Fo"_freeze_importlib.dir\Release\\" /Fd"_freeze_importlib.dir\Release\vc140.pdb" /Gd /TC /errorReport:queue  /Zm200 "C:\D\S4R\Python-3.6.7\Modules\_io\_iomodule.c"
18>  _iomodule.c
18>C:\Program Files (x86)\Windows Kits\10\Include\10.0.17134.0\um\winnt.h(154): fatal error C1189: #error:  "No Target Architecture" [C:\D\S4R\python-build\CMakeBuild\libpython\_freeze_importlib.vcxproj]
18>Done Building Project "C:\D\S4R\python-build\CMakeBuild\libpython\_freeze_importlib.vcxproj" (default targets) -- FAILED.

See full log here: https://1drv.ms/t/s!Arm_AFxB9yqHt54qOQ3phL4bGFYloQ

@jcfr Do you have any advice?

@jcfr
Copy link
Member Author

jcfr commented Apr 12, 2019 via email

@lassoan
Copy link
Contributor

lassoan commented Apr 12, 2019

Thanks for the information. VS2017 is just the IDE, the compiler is VS2015. I built Slicer master branch yesterday on the same computer with the same build options without any problems.

@lassoan
Copy link
Contributor

lassoan commented Apr 12, 2019

Replacing #include <consoleapi.h> by #include <windows.h> in C:\D\S4R\Python-3.6.7\Modules_io_iomodule.c seems to fix the problem. It makes sense, as in general it is not allowed to include internal windows headers in applications but only the top-level windows.h header.

@jcfr
Copy link
Member Author

jcfr commented Apr 12, 2019

A clean build with VS2017 (VS2015 toolset) failed for me on Windows

@lassoan This is fixed in r28138

@jcfr
Copy link
Member Author

jcfr commented Apr 12, 2019

tried building the latest revision using VS2019 (2015 toolset) [... ] There is no python36.lib, but there is a python27.lib in that directory.

Could you confirm this was a clean build ?

@lassoan
Copy link
Contributor

lassoan commented Apr 12, 2019

There are many small issues related to python 2 to 3 switch. Where we should track them? Adding a mantis issue for each would be too much. Should we use a wiki page, online spreadsheet, discourse topic,...?

@jcfr
Copy link
Member Author

jcfr commented Apr 12, 2019

Should we use a wiki page

so far, I have been updating this wiki page: https://www.slicer.org/wiki/Documentation/Labs/Slicer5-roadmap#Python3

@cpinter
Copy link
Member

cpinter commented Apr 12, 2019

Could you confirm this was a clean build ?

Yes it was.

I'll try again from scratch with your new fixes.

@jcfr
Copy link
Member Author

jcfr commented Apr 12, 2019

Could you confirm this was a clean build ?

Yes it was.
I'll try again from scratch with your new fixes.

The fix I integrated only fixes a compilation issue associated with the iomodule, it is strange that a library named python27.lib was created.

Could you check that you are not explicitly setting variable in the Slicer cache. For example, from with a .bat script.

@cpinter
Copy link
Member

cpinter commented Apr 12, 2019

It is indeed strange that a 2.7 lib was created in a clean build!

This is my configure command:

cmake -G "Visual Studio 16 2019" -T "v140" -DQt5_DIR=C:/Qt/Qt5.10.0/5.10.0/msvc2015_64/lib/cmake/Qt5 -DSlicer_USE_SimpleITK:BOOL=ON -DSlicer_USE_PYTHONQT_WITH_OPENSSL:BOOL=ON c:/d/Slicer

@jcfr
Copy link
Member Author

jcfr commented Apr 13, 2019

cmake -G "Visual Studio 16 2019" -T "v140"

worth noting that you are not selecting the Win64 generator

Waiting we update the build system to pass CMAKE_GENERATOR_PLATFORM around (itself initialized by invoking cmake with -A x64), you should specify Visual Studio 16 2019 Win64 instead.


May be worth trying to configure and build only python-cmake-buildsystem:

git clone git://github.com/python-cmake-buildsystem/python-cmake-buildsystem
mkdir python-cmake-buildsystem-build
cd python-cmake-buildsystem-build
cmake -G "Visual Studio 16 2019" -T "v140" -A x64 -DBUILD_LIBPYTHON_SHARED:BOOL=1 ../python-cmake-buildsystem
cmake --build . --config Release -- /m

@cpinter
Copy link
Member

cpinter commented Apr 13, 2019

In the new CMake versions it seems that if you omit the configuration the default will be x64.

This is why I think it will be:
image

@cpinter
Copy link
Member

cpinter commented Apr 13, 2019

Also I can confirm that Visual Studio is actually building Slicer in 64 bits.

@jcfr
Copy link
Member Author

jcfr commented Apr 13, 2019

Since i don't know if the initialization to default value happen in the gui dialog or in the regular CMake code, I don't know if the platform is correctly initialized by default in sub project. Could you check that CMAKE_GENERATOR_PLATFORM is correctly set in a sub project ?

@jcfr
Copy link
Member Author

jcfr commented Apr 13, 2019

re: restart() issue

@lassoan This is related to the auto-completion and is independent of the function being executed. If the function is auto-completed, it will not be evaluated.

@cpinter
Copy link
Member

cpinter commented Apr 13, 2019

CMAKE_GENERATOR_PLATFORM is empty.

But as I said Visual Studio is building in 64 bits.
Also
CMAKE_CXX_COMPILER:FILEPATH=C:/Program Files (x86)/Microsoft Visual Studio 14.0/VC/bin/amd64/cl.exe
CMAKE_CXX_COMPILER:FILEPATH=C:/Program Files (x86)/Microsoft Visual Studio 14.0/VC/bin/amd64/cl.exe
CMAKE_C_COMPILER:FILEPATH=C:/Program Files (x86)/Microsoft Visual Studio 14.0/VC/bin/amd64/cl.exe

@cpinter
Copy link
Member

cpinter commented Apr 13, 2019

@jcfr This time the build succeeded.

I am 100% sure that it was a clean build for the first time, and that python27 thing puzzles me too. If it happens again I'll let you know. Thanks!

@jcfr
Copy link
Member Author

jcfr commented Apr 13, 2019 via email

@jcfr
Copy link
Member Author

jcfr commented Apr 15, 2019

re: restart() issue

@lassoan This is related to the auto-completion and is independent of the function being executed. If the function is auto-completed, it will not be evaluated.

This will be fixed by commontk/CTK#859

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants