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

Updates to make smart-dispatch Python 3 compatible #161

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

jfsantos
Copy link

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 of file 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).

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 94.473% when pulling c8e697d on jfsantos:master into c94f08e on SMART-Lab:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 94.473% when pulling 2d97763 on jfsantos:master into c94f08e on SMART-Lab:master.

@jfsantos
Copy link
Author

There are just a few issues on Travis, I'm going to check them out soon.

Copy link
Member

@MarcCote MarcCote left a 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'],
Copy link
Member

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')
Copy link
Member

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?

Copy link
Author

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.

@MarcCote MarcCote mentioned this pull request Mar 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants