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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants