-
Notifications
You must be signed in to change notification settings - Fork 18
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
Updates to make smart-dispatch Python 3 compatible #161
base: master
Are you sure you want to change the base?
Conversation
There are just a few issues on Travis, I'm going to check them out soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. future
seems better than `six. Just have to make sure the tests are updated accordingly.
@@ -14,6 +14,6 @@ | |||
license='LICENSE.txt', | |||
description='An easy to use job launcher for supercomputers with PBS compatible job manager.', | |||
long_description=open('README.md').read(), | |||
install_requires=['psutil>=1'], | |||
install_requires=['psutil>=1', 'future'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't know about that library. Neat!
|
||
try: | ||
_unicode = unicode | ||
_utf8 = lambda x: _unicode(x, 'UTF-8') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have thought this sort of "hacks" isn't needed anymore thanks to the future
library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I did this before adding future
as a dependency because I thought I could get around it without additional dependencies... until I got to the tests. I'll change it to use future.builtins.str
instead.
I made a few updates to make smart-dispatch compatible with Python 3, namely replacing all print statement calls by the print function, converting a bytestring to a string and using
open
instead offile
to detect if the argument of the option-f
is a file. I did not do extensive testing yet but this does not break it with Python 2 or 3 when running a basic test (smart_dispatch.py -qtest launch nvidia-smi
).