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

[FIX] PY3 compatibility revisited #1572

Merged
merged 136 commits into from Sep 7, 2016

Conversation

oesteban
Copy link
Contributor

@oesteban oesteban commented Aug 9, 2016

No description provided.

@@ -0,0 +1,378 @@
[MASTER]
Copy link
Member

Choose a reason for hiding this comment

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

accidental inclusion of this file

@coveralls
Copy link

coveralls commented Aug 10, 2016

Coverage Status

Coverage decreased (-0.2%) to 72.041% when pulling f38fb07 on oesteban:fix/Py3UseBuiltinOpen into a26ef19 on nipy:master.

@oesteban
Copy link
Contributor Author

oesteban commented Aug 10, 2016

Ok, we are growing too big here.

So, this is the situation: some parts of nipype are not python 3 compatible. The were passing unnoticed because the tests did not exercise these parts and CircleCI smoke tests were done in python 2.7.

In this PR, I changed the tests in CircleCI to run in python 3.5, with the following chain of issues:

  • First problem were open() calls. I had to add a from builtins import open in all files using it.
  • Now, these files would read/write unicode -> add from __future__ import unicode_literals.
  • Many interfaces stopped working. Particularly those using Directory and DictStrStr. I fixed those, making Directory derive from traits.Unicode and DictStrStr with six.string_types. I also disabled the fast C type checker for them.
  • I removed the external code of six. I don't see the point to maintain our version in parallel.

Finally, after too many modifications, the big issue is doctests. Python 2 will want to have a u-strings in tests, while Python 3 does not. There are two solutions:

  1. Migrate to py.test (and enable ALLOW_UNICODE). However the test generators will not work, and we have plenty of those.
  2. Tricks to fix the docstrings before nose:
  3. (EDITED) Use doctest-ignore-unicode 👍

Any opinions, @satra?

@coveralls
Copy link

coveralls commented Sep 7, 2016

Coverage Status

Coverage decreased (-0.002%) to 72.269% when pulling 0921506 on oesteban:fix/Py3UseBuiltinOpen into 0f2cb9f on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 72.263% when pulling 38cbf46 on oesteban:fix/Py3UseBuiltinOpen into 4eb197f on nipy:master.

7 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 72.263% when pulling 38cbf46 on oesteban:fix/Py3UseBuiltinOpen into 4eb197f on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 72.263% when pulling 38cbf46 on oesteban:fix/Py3UseBuiltinOpen into 4eb197f on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 72.263% when pulling 38cbf46 on oesteban:fix/Py3UseBuiltinOpen into 4eb197f on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 72.263% when pulling 38cbf46 on oesteban:fix/Py3UseBuiltinOpen into 4eb197f on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 72.263% when pulling 38cbf46 on oesteban:fix/Py3UseBuiltinOpen into 4eb197f on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 72.263% when pulling 38cbf46 on oesteban:fix/Py3UseBuiltinOpen into 4eb197f on nipy:master.

@coveralls
Copy link

coveralls commented Sep 7, 2016

Coverage Status

Coverage decreased (-0.002%) to 72.263% when pulling 38cbf46 on oesteban:fix/Py3UseBuiltinOpen into 4eb197f on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 72.263% when pulling 644aa54 on oesteban:fix/Py3UseBuiltinOpen into 4eb197f on nipy:master.

7 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 72.263% when pulling 644aa54 on oesteban:fix/Py3UseBuiltinOpen into 4eb197f on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 72.263% when pulling 644aa54 on oesteban:fix/Py3UseBuiltinOpen into 4eb197f on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 72.263% when pulling 644aa54 on oesteban:fix/Py3UseBuiltinOpen into 4eb197f on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 72.263% when pulling 644aa54 on oesteban:fix/Py3UseBuiltinOpen into 4eb197f on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 72.263% when pulling 644aa54 on oesteban:fix/Py3UseBuiltinOpen into 4eb197f on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 72.263% when pulling 644aa54 on oesteban:fix/Py3UseBuiltinOpen into 4eb197f on nipy:master.

@coveralls
Copy link

coveralls commented Sep 7, 2016

Coverage Status

Coverage decreased (-0.002%) to 72.263% when pulling 644aa54 on oesteban:fix/Py3UseBuiltinOpen into 4eb197f on nipy:master.

@oesteban
Copy link
Contributor Author

oesteban commented Sep 7, 2016

this is finally ready!. It is very worrying that the MultiProc plugin is about twice as slow as the Linear plugin... I have to disable the resource profiler to discard it as bottleneck (in another PR).

@satra
Copy link
Member

satra commented Sep 7, 2016

the resource profiler should be disabled in current master

@satra
Copy link
Member

satra commented Sep 7, 2016

shall we merge this? it seems like a lot of things are dependent on this now.

@oesteban
Copy link
Contributor Author

oesteban commented Sep 7, 2016

@satra - I think so. Regarding the resource profiler: I will investigate the issue. Do you think that could be the explanation for MultiProc taking so much longer?

- docker run -e FSL_COURSE_DATA="/root/examples/nipype-fsl_course_data" -v /etc/localtime:/etc/localtime:ro -v ~/scratch:/scratch -w /scratch nipype/nipype_test:py27 /usr/bin/run_nosetests.sh :
timeout: 2600
- docker run -v /etc/localtime:/etc/localtime:ro -v ~/examples:/root/examples:ro -v ~/scratch:/scratch -w /scratch nipype/nipype_test:py35 /usr/bin/run_examples.sh fmri_fsl_reuse Linear /root/examples/ level1_workflow
- docker run -v /etc/localtime:/etc/localtime:ro -e NIPYPE_NUMBER_OF_CPUS=4 -v ~/examples:/root/examples:ro -v ~/scratch:/scratch -w /scratch nipype/nipype_test:py27 /usr/bin/run_examples.sh fmri_spm_nested MultiProc /root/examples/ level1
Copy link
Member

Choose a reason for hiding this comment

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

Where is the NIPYPE_NUMBER_OF_CPUS variable used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I see!

On Wed, Sep 7, 2016 at 11:41 AM, Oscar Esteban notifications@github.com
wrote:

In circle.yml
#1572 (comment):

     timeout: 1600
    • docker run -v /etc/localtime:/etc/localtime:ro -v ~/examples:/root/examples:ro -v $(pwd)/scratch:/scratch -w /scratch nipype/nipype_test:py27 /usr/bin/run_examples.sh fmri_fsl_reuse Linear /root/examples/ level1_workflow
    • docker run -v /etc/localtime:/etc/localtime:ro -v ~/examples:/root/examples:ro -v $(pwd)/scratch:/scratch -w /scratch nipype/nipype_test:py27 /usr/bin/run_examples.sh fmri_spm_nested Linear /root/examples/ level1
    • docker run -v /etc/localtime:/etc/localtime:ro -v ~/examples:/root/examples:ro -v $(pwd)/scratch:/scratch -w /scratch nipype/nipype_test:py27 /usr/bin/run_examples.sh fmri_spm_nested Linear /root/examples/ l2pipeline
    • docker run -e FSL_COURSE_DATA="/root/examples/nipype-fsl_course_data" -v /etc/localtime:/etc/localtime:ro -v ~/scratch:/scratch -w /scratch nipype/nipype_test:py27 /usr/bin/run_nosetests.sh :
  •    timeout: 2600
    
    • docker run -v /etc/localtime:/etc/localtime:ro -v ~/examples:/root/examples:ro -v ~/scratch:/scratch -w /scratch nipype/nipype_test:py35 /usr/bin/run_examples.sh fmri_fsl_reuse Linear /root/examples/ level1_workflow
    • docker run -v /etc/localtime:/etc/localtime:ro -e NIPYPE_NUMBER_OF_CPUS=4 -v ~/examples:/root/examples:ro -v ~/scratch:/scratch -w /scratch nipype/nipype_test:py27 /usr/bin/run_examples.sh fmri_spm_nested MultiProc /root/examples/ level1

https://github.com/oesteban/nipype/blob/fix/Py3UseBuiltinOpen/tools/run_
examples.py#L23


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/nipy/nipype/pull/1572/files/644aa549fd478afb50419774e3238eab0c08ab93#r77880468,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAOkp4eDjPttM0KyEuGRHIgJO0KyZRFRks5qnwV0gaJpZM4Jfl9M
.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was the way to set it in run_examples.py with the least changes.

@chrisgorgo chrisgorgo merged commit d12150d into nipy:master Sep 7, 2016
@oesteban oesteban deleted the fix/Py3UseBuiltinOpen branch September 7, 2016 18:52
@oesteban oesteban mentioned this pull request Sep 7, 2016
oesteban added a commit to oesteban/nipype that referenced this pull request Sep 12, 2016
Found corner cases that failed when calculating the hash of dicts - fixes nipy#1620.

Also found that list of lists of ints do not work in nipy#1591. Fixes also that one.
oesteban added a commit that referenced this pull request Sep 14, 2016
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.

None yet

6 participants