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

getting ready for python 3 - part 2 #656

Open
wants to merge 3 commits into
base: devel
from

Conversation

Projects
None yet
2 participants
@adhintz
Copy link
Contributor

commented May 22, 2019

I've tested this PR with the docker test and a real drive.

adhintz added some commits May 21, 2019

Replace xrange with range
python 2 to 3: https://portingguide.readthedocs.io/en/latest/iterators.html#new-behavior-of-range

Where there's a chance of performance issues with switching from the python 2 xrange to python 2 range, I've imported `from six.moves import range` to preserve the iterator behavior.
However in places where there's not a performance issue, I've left out the six import so that there's less code churn after switching to python 3.
@geohot

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2019

We are currently building a new /usr with python 3.7 included that will ship with NEOS 10.

Whether we migrate comes down to is CPU usage. Python 3 = more, no. Python 3 = less, yes. Python 3 = same, maybe.

After we ship new /usr, if you could port and benchmark a single service and compare, that would be great. controlsd uses less CPU = we will commit to Python 3.

@geohot geohot added the refactor label Jun 7, 2019

@@ -8,6 +8,7 @@
create_fcw_command, create_ui_command
)
from common.realtime import sec_since_boot
from six.moves import range

This comment has been minimized.

Copy link
@geohot

geohot Jun 7, 2019

Contributor

I don't like six, why do we need it here?

@@ -7,20 +7,21 @@
from common.realtime import sec_since_boot
from cereal import log
import unittest
from six.moves import range

This comment has been minimized.

Copy link
@geohot

geohot Jun 7, 2019

Contributor

For tests, I don't think creating the list matters

@geohot

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Can we remove six? I think a lot of those ranges are in tests. If so, we are good to merge after rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.