Skip to content

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.

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

3 participants