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

Move to Python 3: dependencies status #2891

Closed
arikfr opened this issue Oct 3, 2018 · 27 comments
Closed

Move to Python 3: dependencies status #2891

arikfr opened this issue Oct 3, 2018 · 27 comments

Comments

@arikfr
Copy link
Member

arikfr commented Oct 3, 2018

Thanks to the work of @cclauss our code is (more) ready for Python 3. I also ran caniusepython3 on our requirements files and the results are very encouraging:

$ caniusepython3 --requirements requirements.txt
Finding and checking dependencies ...

You need 5 projects to transition to Python 3.
Of those 5 projects, 4 have no direct dependencies blocking their transition:

  chromelogger
  flask-sslify
  python-geoip (which is blocking python-geoip-geolite2)
  restrictedpython
  • python-geoip has a Python 3 compatible alternative (python-geoip-python3)
  • Latest master version of flask-sslify is marked as Python 3 compatible, but no new version was released for a long time.
  • chromelogger is unknown but we can either drop it or update to support Python 3 (~140 lines of code).
  • restrictedpython has an unreleased version 4 that supports Python 3.
$ caniusepython3 --requirements requirements_all_ds.txt
Finding and checking dependencies ...

You need 1 project to transition to Python 3.
Of that 1 project, 1 has no direct dependencies blocking its transition:

  pyhive

I'm not sure why pyhive is marked as not supported, because it is. Maybe we just need to update the version.

$ caniusepython3 --requirements requirements_dev.txt
Finding and checking dependencies ...

You need 1 project to transition to Python 3.
Of that 1 project, 1 has no direct dependencies blocking its transition:

  pytest-watch

I'm not sure about the state of pytest-watch, but we can always remove it.

@cclauss
Copy link
Contributor

cclauss commented Oct 3, 2018

PyHive has no trove classifiers in its PyPI record... https://pypi.org/project/PyHive

But it is being tested under all current versions of CPython https://github.com/dropbox/PyHive/blob/master/.travis.yml#L8

dropbox/PyHive#243

@cclauss
Copy link
Contributor

cclauss commented Oct 3, 2018

Same with pytest-watch... https://pypi.org/project/pytest-watch

joeyespo/pytest-watch#95

@jezdez
Copy link
Member

jezdez commented Oct 4, 2018

This is super exciting, didn't realize how close we'd be in terms of dependencies.

For flask-sslify I planned to work on mozilla#562 and port Redash to use flask-talisman which also supports enforcing encrypted connections (on top of plenty of other security related things, which is what interests us at Mozilla). A bit like a kill two birds with one stone situation. Any general objections about this?

@arikfr
Copy link
Member Author

arikfr commented Oct 4, 2018

@jezdez sounds good to me. 👍

@truebit
Copy link

truebit commented Jan 4, 2019

I saw many ImportError when trying to run on python3.
Is there any issue to locate those incompatible codes for Python3?

@jezdez
Copy link
Member

jezdez commented May 15, 2019

@arikfr Oh btw, we merged flask-talisman! Which means there shouldn't be any blocker to this?

@arikfr
Copy link
Member Author

arikfr commented May 15, 2019

@jezdez We should probably run caniusepython3 again to review if anything changed.

But there are probably other things that are blocking switching over to Python 3 in our code or dependencies. I'm just not sure what they are. 😳

@jezdez
Copy link
Member

jezdez commented May 15, 2019

$ caniusepython3 -r requirements.txt
Finding and checking dependencies ...

You need 3 projects to transition to Python 3.
Of those 3 projects, 2 have no direct dependencies blocking their transition:

  chromelogger
  python-geoip (which is blocking python-geoip-geolite2)
$ caniusepython3 -r requirements_all_ds.txt
Finding and checking dependencies ...

You need 3 projects to transition to Python 3.
Of those 3 projects, 3 have no direct dependencies blocking their transition:

  ibm-db
  pydruid
  pyhive
$ caniusepython3 -r requirements_dev.txt
Finding and checking dependencies ...

🎉  You have 0 projects blocking you from using Python 3!

@arikfr
Copy link
Member Author

arikfr commented May 15, 2019

The requirements.txt check says 3 projects, but lists 2. Any idea why?

@jezdez
Copy link
Member

jezdez commented May 15, 2019

I think it counts python-geoip-geolite2 and python-geoip individually:

caniusepython3 -p python-geoip-geolite2
Finding and checking dependencies ...

You need 2 projects to transition to Python 3.
Of those 2 projects, 1 has no direct dependencies blocking its transition:

  python-geoip (which is blocking python-geoip-geolite2)

@jezdez
Copy link
Member

jezdez commented May 15, 2019

IBM-db is tested under Py2 and Py3 https://github.com/ibmdb/python-ibmdb/blob/master/.travis.yml

I've opened ibmdb/python-ibmdb#395 to fix caniusepython3 missing this.

@arikfr
Copy link
Member Author

arikfr commented May 15, 2019

pydruid is also tested with Python 3.6: https://github.com/druid-io/pydruid/blob/master/.travis.yml

@arikfr
Copy link
Member Author

arikfr commented May 15, 2019

Based on this, it looks like the dependencies are probably a non issue. Now there is only the small issue of figuring out if our code works as intended on Python 3 😅

@cclauss
Copy link
Contributor

cclauss commented May 15, 2019

My sense is that you should try putting apt install python3 python3-pip into your CirceCI builds and see where the tests fail. I think you are really close and it would be good to understand what is failing.

@jezdez
Copy link
Member

jezdez commented May 15, 2019

pydruid is also tested with Python 3.6: druid-io/pydruid:.travis.yml@master

Opened druid-io/pydruid#164

@jezdez
Copy link
Member

jezdez commented May 15, 2019

Yeah, running 2to3 (or some wrappers mentioned on https://docs.python.org/3/howto/pyporting.html) make sense as well.

There is a pretty good high-level overview in The Conservative Python 3 Porting Guide which I think applies to Redash well.

@cclauss
Copy link
Contributor

cclauss commented May 15, 2019

I would avoid 2to3 because it tends to break Py2 compatibility to get to Py3 compatibility.

I would recommend using either futurize or modernize instead. These thin wrappers on top of 2to3 that preserve compatibility with bothPy2 and Py3

@arikfr
Copy link
Member Author

arikfr commented May 15, 2019

I'm not sure we will support Python 2 once we move to 3.

@cclauss
Copy link
Contributor

cclauss commented May 15, 2019

Your call. However best practice is to run them both in a single codebase for at least a little time because the A/B testing can be invaluable. See the conservative guild and python docs on porting.

@cclauss
Copy link
Contributor

cclauss commented May 15, 2019

A few Python 3 issues to watch out for in the porting...

flake8 testing of https://github.com/getredash/redash on Python 3.7.1

$ flake8 . --count --select=E9,F63,F72,F82 --show-source --statistics

./redash/query_runner/__init__.py:290:8: F821 undefined name 'unicode'
    if unicode(string_value).lower() in ('true', 'false'):
       ^
./redash/query_runner/drill.py:29:16: F821 undefined name 'unicode'
        return unicode(string_value).lower() == 'true'
               ^
./redash/query_runner/drill.py:34:12: F821 undefined name 'unicode'
    return unicode(string_value)
           ^
./redash/models/parameterized_query.py:16:48: F821 undefined name 'unicode'
    return {"name": row[name_column], "value": unicode(row[value_column])}
                                               ^
./redash/models/parameterized_query.py:122:53: F821 undefined name 'basestring'
            "text": lambda value: isinstance(value, basestring),
                                                    ^
./redash/models/parameterized_query.py:125:36: F821 undefined name 'unicode'
            "query": lambda value: unicode(value) in [v["value"] for v in dropdown_values(definition["queryId"])],
                                   ^
./redash/utils/__init__.py:99:28: F821 undefined name 'buffer'
        elif isinstance(o, buffer):
                           ^
./redash/authentication/saml_auth.py:116:12: F632 use ==/!= to compare str, bytes, and int literals
        if key is 'Location':
           ^
./tests/test_utils.py:47:37: F821 undefined name 'buffer'
        self.assertEqual(json_dumps(buffer("test")), '"74657374"')
                                    ^
1     F632 use ==/!= to compare str, bytes, and int literals
8     F821 undefined name 'unicode'
9

@arikfr
Copy link
Member Author

arikfr commented May 15, 2019

I setup a new CircleCI job to run tests on Python 3. It uncovered that our MySQL lib does not support Python 3:

https://circleci.com/gh/getredash/redash/21775

I updated to mysqlclient and waiting for a new build:

https://circleci.com/gh/getredash/redash/21782

--

Update: new build fails too. I will iterate on the dependencies locally.

@cclauss
Copy link
Contributor

cclauss commented May 15, 2019

See the comments at the bottom of mitsuhiko/python-geoip#5

@arikfr
Copy link
Member Author

arikfr commented May 15, 2019

Python 2 support after Python 3: I guess it depends on how much effort it will require. If possible we will keep support for both.

Flake 8 tests: I assumed that the CircleCI job for Flake8 tests on Python 3 will fail if it doesn't pass 😳 Oops.

@cclauss
Copy link
Contributor

cclauss commented May 15, 2019

https://stackoverflow.com/questions/4960048/how-can-i-connect-to-mysql-in-python-3-on-windows

@arikfr
Copy link
Member Author

arikfr commented May 15, 2019

There are a few more dependencies that fail to build with the Python 3 image.

I noticed caniusepython3 maintains a list of projects that have Python 3 alternatives, like mysql-python -> mysqlclient. It could be nice if it mentioned this in the output...

Update: it has a verbose output that should list them -- will check.

@jezdez
Copy link
Member

jezdez commented May 15, 2019

Regarding whether to go 2/3 or straight to 3, I don't think it matters which strategy to use as long as we can run on Python 3 before the end of the year -- so chose the strategy that works best for you.

FWIW I know a few distributors will support Python 2 going forward, but it'd cost us, which I don't think is a viable option either for Redash SaaS or the OSS users. TBH I fully expect to see Python 2 vulnerabilities to show up after the EOL.

Handy tool: https://pythonclock.org/

@arikfr
Copy link
Member Author

arikfr commented Apr 3, 2023

This is not related to the issue, but I assume that the issue author or followers might have SAML enabled for their deployment and should be aware of the following Security Advisory: #5961. This affects all Redash versions and should be patched immediately.

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

No branches or pull requests

4 participants