Skip to content

Fix dictstrstr #1629

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

Merged
merged 3 commits into from
Sep 15, 2016
Merged

Fix dictstrstr #1629

merged 3 commits into from
Sep 15, 2016

Conversation

bpinsard
Copy link
Contributor

The future strings in traits definition causes crashes for most interfaces using os.environ as an input.

@@ -33,7 +33,7 @@
from traits.api import BaseUnicode
from traits.api import Unicode

DictStrStr = traits.Dict(str, (bytes, str))
DictStrStr = traits.Dict((bytes,str,unicode), (bytes,str,unicode))
Copy link
Contributor

Choose a reason for hiding this comment

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

unicode does not exist in python 3 anymore.

you can do

import sys

DictStrStr = traits.Dict(str, (bytes,str))
if sys.version_info[0] < 3:
    DictStrStr = traits.Dict((bytes,str,unicode), (bytes,str,unicode))

However, DictStrStr = traits.Dict((bytes, str), (bytes,str)) does not solve your problem as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the error, it seems that all my environ strings are in unicode, so without unicode it does not solve my problem.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the solution is to typecast the environ somewhere in the base
interface class?

On Wed, Sep 14, 2016 at 9:00 AM, bpinsard [email protected] wrote:

In nipype/interfaces/traits_extension.py
#1629 (comment):

@@ -33,7 +33,7 @@
from traits.api import BaseUnicode
from traits.api import Unicode

-DictStrStr = traits.Dict(str, (bytes, str))
+DictStrStr = traits.Dict((bytes,str,unicode), (bytes,str,unicode))

In the error, it seems that all my environ strings are in unicode, so
without unicode it does not solve my problem.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/nipy/nipype/pull/1629/files/fce449863911f9512a69a35c2ae93eabb941ef8a#r78779207,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAOkp3oMdKPi2qiLTGg8QpRyp9K2DvBKks5qqBopgaJpZM4J8Jid
.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does __future__ or something else is changing the environ content?
Because in any other context, the environ is all defined in str (=python3 bytes).

Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently, the problem is in the keys of environ, not the values.

Did it work for you using DictStrStr = traits.Dict((bytes, str), (bytes,str)) instead the version with unicode?

(i.e. adding bytes for the validation of keys in DictStrStr)

This should do (it worked on the SGE environment I created for testing, where I could reproduce the issue)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without unicode, it does not work, weirdly my os.environ is all unicode inside nipype, outside it regular python str.
What is weird, I removed my bashrc, then the bug is fixed, I also try to remove a lot of stuff from my bashrc and it is also fixed.
The bashrc is not utf8 coded, and does not contains any special character, so it might be some sourcing that introduce this bug.
So the interaction of these sourced bash scripts with nipype and future is causing os.environ to be all unicode in nipype only, why, I do not know?
Anyway, thanks for the help, I think we can keep the DictStrStr = traits.Dict((bytes, str), (bytes,str)) then ?

@oesteban oesteban added the bug label Sep 13, 2016
@oesteban
Copy link
Contributor

Ok just confirmed #1621 in a docker image with SGE.

Also confirmed that DictStrStr = traits.Dict((bytes, str), (bytes,str)) fixes the issue.

@codecov-io
Copy link

codecov-io commented Sep 14, 2016

Current coverage is 71.08% (diff: 100%)

Merging #1629 into master will increase coverage by 0.21%

@@             master      #1629   diff @@
==========================================
  Files          1020       1020           
  Lines         51240      52343   +1103   
  Methods           0          0           
  Messages          0          0           
  Branches       7259       7645    +386   
==========================================
+ Hits          36314      37206    +892   
- Misses        13840      14030    +190   
- Partials       1086       1107     +21   

Powered by Codecov. Last update a746633...78adbc0

@satra
Copy link
Member

satra commented Sep 15, 2016

@bpinsard - it's possible that you have some environmental setup in bashrc that's affecting things.

see: https://perlgeek.de/en/article/set-up-a-clean-utf8-environment

also, there are lines like this, which can affect input:

https://github.com/nipy/nipype/blob/master/nipype/interfaces/base.py#L1229

@oesteban - is bytes needed in DictStrStr

@oesteban
Copy link
Contributor

@satra - yes, I think this patch is necessary.

@oesteban oesteban merged commit 0bb9561 into nipy:master Sep 15, 2016
@satra satra removed the in-progress label Sep 15, 2016
@bpinsard bpinsard deleted the fix_dictstrstr branch September 15, 2016 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants