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

Add python 3 support. #126

Merged
merged 11 commits into from Aug 1, 2017
Merged

Add python 3 support. #126

merged 11 commits into from Aug 1, 2017

Conversation

gas1121
Copy link
Contributor

@gas1121 gas1121 commented Jul 16, 2017

Use decode_responses option on Redis client and value_deserializer,value_serializer option on Kafka client to handle unicode problem. Also fix several syntax error and update several test cases for python 3. And as scrapy-cluster 1.2 use ujson instead of pickle, I think no migration is needed.

…n on Redis client and

value_deserializer,value_serializer option on Kafka client to handle unicode problem. We also
fix several syntax error and update several test cases.
@coveralls
Copy link

coveralls commented Jul 16, 2017

Coverage Status

Coverage increased (+0.0008%) to 65.79% when pulling 12a9da5 on gas1121:py3 into dcbd36d on istresearch:dev.

@madisonb
Copy link
Collaborator

Wow this is great! I had no idea I was actually so close to getting this project python 3 compatible, your changes make it seem so simple.

This has been a long standing feature request, and given that the travis build is 👍 the only thing I would like to do is some internal validation and run it myself - the code looks good.

Fixes #46

@coveralls
Copy link

coveralls commented Jul 21, 2017

Coverage Status

Coverage increased (+0.002%) to 65.791% when pulling 2f3ec46 on gas1121:py3 into dcbd36d on istresearch:dev.

@gas1121
Copy link
Contributor Author

gas1121 commented Jul 21, 2017

fix #128 It is an unicode issue as Kazoo client requires and returns byte type value when using get and set. Also fix related tests and add an online test for ZookeeperWatcher.

@madisonb
Copy link
Collaborator

Newest changes also look good. The only thing I see missing here is that we may need to add naive os tests, since ./run_docker_test.sh scripts don't actually test the scutils library offline/online tests (they only test the particular component).

Its an older artifact from older versions of scrapy cluster, but in past we used the ansible roles to deploy and ensure the project could run on both native ubuntu and centos. In a perfect world, I would like to duplicate them but for python3.6 instead. Those ansible tests run the whole offline and online test suite, which would pick your changes.

That won't prevent me from merging this in though, but I may make a separate ticket to address it.

@madisonb
Copy link
Collaborator

madisonb commented Jul 23, 2017

If you run the utilities unit tests, both in python 2 and python 3 the unit tests fail at various stages. The scutils library is pretty critical for functionality across the project, so failing tests here means a project that becomes broken.

For python 2 (/utils):

$ nosetests -v --with-coverage --cover-erase # ok

$ python tests/online.py -r localhost -p 6379 # fails
test_fifo_queue (__main__.TestRedisFifoQueue) ... ERROR
test_priority_queue (__main__.TestRedisPriorityQueue) ... ERROR

For python 3 (/utils):

$ nosetests -v --with-coverage --cover-erase # fails
test_decode (test_redis_queue.TestBase) ... ERROR
test_encode (test_redis_queue.TestBase) ... FAIL

$ python tests/online.py -r localhost -p 6379 # fails
test_fifo_queue (__main__.TestRedisFifoQueue) ... ERROR
test_priority_queue (__main__.TestRedisPriorityQueue) ... ERROR
test_stack (__main__.TestRedisStack) ... ERROR
test_get_file_contents (__main__.TestZookeeperWatcher) ... Exception in thread Thread-30:
Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/threading.py", line 916, in _bootstrap_inner
    self.run()
  File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/threading.py", line 864, in run
    self._target(*self._args, **self._kwargs)
  File "/Users/madisonb/.local/share/virtualenvs/scrapy-cluster-p36/lib/python3.6/site-packages/scutils/zookeeper_watcher.py", line 121, in init_connections
    self.update_file(self.my_file)
  File "/Users/madisonb/.local/share/virtualenvs/scrapy-cluster-p36/lib/python3.6/site-packages/scutils/zookeeper_watcher.py", line 216, in update_file
    self.update_pointed()
  File "/Users/madisonb/.local/share/virtualenvs/scrapy-cluster-p36/lib/python3.6/site-packages/scutils/zookeeper_watcher.py", line 245, in update_pointed
    watch=self.watch_pointed)
  File "/Users/madisonb/.local/share/virtualenvs/scrapy-cluster-p36/lib/python3.6/site-packages/kazoo/client.py", line 1026, in get
    return self.get_async(path, watch).get()
  File "/Users/madisonb/.local/share/virtualenvs/scrapy-cluster-p36/lib/python3.6/site-packages/kazoo/client.py", line 1036, in get_async
    raise TypeError("Invalid type for 'path' (string expected)")
TypeError: Invalid type for 'path' (string expected)
# did not get any further because the thread gets stuck

I triple checked and made sure I had the latest build from this PR as the scutils package installed in my virtualenv, can you reproduce what I am seeing?

UPDATE: I think I see why this passed on travis, its because when I cut a release I removed the code that installs bleeding edge scutils from the unit tests... let me add that back in to the dev branch and I will add a new comment

@madisonb
Copy link
Collaborator

The important part here is this section of code that runs within the ansible travis tests. It not only uninstalls pypi scutils, but installs the bleeding edge source and then runs the full suite of offline and online unit tests. (of which we don't get in the docker suite tests)

This could be caught in a variety of ways:

  1. Add a special docker container for running the scutils library only. Then we could use ./run_docker_tests.sh and it would run the scutils unit tests and integration tests via our normal docker compose setup.
  2. Add ubuntu/centos python 3 ansible roles. I think this is a worthwhile effort for folks who don't use docker, and would help ensure the native source code can run in normal environments as well.

Both 1 and 2 above seem like good ideas and would help catch nagging scutils library updates that may arise within this PR.

@gas1121
Copy link
Contributor Author

gas1121 commented Jul 24, 2017

I've merged the newest commit and my travis build seems work fine. Also before I push my update I've passed all tests on my local docker environment.
Did you update both code and tests and delete all old test datas in redis? Old redis data may also make tests fail. And as I added zookeeper online test into util's online test, zookeeper's host may also need to be specified.
I agree new travis test job for python 3 is needed. My work flow to test scutils on my local docker environment is similar with the first one and I think I can push an update about this.

@coveralls
Copy link

coveralls commented Jul 24, 2017

Coverage Status

Coverage increased (+0.002%) to 65.791% when pulling e972b0e on gas1121:py3 into 93d7ad5 on istresearch:dev.

@madisonb
Copy link
Collaborator

I have not been able to throw anything at this PR that did not pass all of my tests. I plan on merging this 🤞 tomorrow. Awesome work @gas1121!

@madisonb madisonb merged commit e972b0e into istresearch:dev Aug 1, 2017
@gas1121 gas1121 deleted the py3 branch August 14, 2017 02:51
@dyangelo-grullon
Copy link

I'm just passing by. Is python 3 supported now? The README claims that python 2.7 is required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants