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

Migrate to Python 3 #4181

Closed
arikfr opened this issue Sep 24, 2019 · 19 comments
Closed

Migrate to Python 3 #4181

arikfr opened this issue Sep 24, 2019 · 19 comments

Comments

@arikfr
Copy link
Member

arikfr commented Sep 24, 2019

As Python 2 reaches EOL, we're planning to have the next release of Redash to support Python 3.

Because Python 2 reaches EOL and the fact we don't have capacity to support both versions, once we switch to Python 3 we will support Python 3 only.

@arikfr
Copy link
Member Author

arikfr commented Sep 24, 2019

Related: #2891.
Previous attempt: https://github.com/getredash/redash/tree/py3-tests.

@arikfr
Copy link
Member Author

arikfr commented Sep 24, 2019

We will probably need to update some dependencies (either versions or replace) as outlined in #2891. But as we will be carefully testing everything, it's worth bumping the versions of other dependencies like moving to Flask 1.0+.

@arikfr arikfr pinned this issue Sep 24, 2019
@arikfr arikfr changed the title Switch to Python 3 Migrate to Python 3 Sep 24, 2019
@NicolasLM
Copy link
Contributor

Hi, here is the summary of my progress on this issue for the week:

  • The most obvious syntax changes have been addressed throughout the code base, mostly using 2to3
  • The Dockerfile has been updated to use Python 3.7
  • The frontend build in CircleCI has been changed to also use Python 3
  • A few dependencies have been upgraded because they were not compatible with Python 3. I tried to avoid upgrading unnecessary dependencies since I want to isolate the issues related to Python 3 from more generic backward incompatibilities that are often introduced in new versions of a package.
  • I am working my way through the tests, fixing the broken ones. The vast majority of them already pass.

Currently the web app and the tasks work fine for me on Python 3. I have not encountered any error using the application locally, but I have only tested the Postgres and MySQL datasources. Feel free to give it a try if you want, the code is in the python-3 branch.

@arikfr
Copy link
Member Author

arikfr commented Oct 5, 2019

Thank you for the detailed update, @NicolasLM. 👍

@NicolasLM
Copy link
Contributor

Only a single test remains broken: test_outdated_queries_works_scheduled_queries_tracker. After spending some time on it I feel that the migration to Python 3 uncovered a logic error in the test.
I am not sure which behavior is expected in the test as fixing the obvious comparison between TZ aware and naive datetime, makes the actual assert fail.

Besides that I also stopped using the CSV UnicodeWriter. I think that the clear distinction of str vs bytes in Python 3 makes this class redundant. If you could tell me why this class was needed in the first place I could verify that it is indeed not required anymore.

@arikfr
Copy link
Member Author

arikfr commented Oct 10, 2019

Only a single test remains broken: test_outdated_queries_works_scheduled_queries_tracker. After spending some time on it I feel that the migration to Python 3 uncovered a logic error in the test.

I think you're right. I changed the test to use redash.utils.utcnow as the other tests in this group (#4230).

Besides that I also stopped using the CSV UnicodeWriter. I think that the clear distinction of str vs bytes in Python 3 makes this class redundant. If you could tell me why this class was needed in the first place I could verify that it is indeed not required anymore.

I believe that without it trying to write unicode into CSV failed. The implementation of UnicodeWriter was taken from csv docs for Python 2. I assume this is not needed anymore.

@NicolasLM
Copy link
Contributor

Hi, regarding the port of the code to Python 3:

  • All tests pass on Python 3
  • The most common features of the application seem to work well.

I think that the code is ready for a review and thorough testing (specifically regarding data sources). Let me know if you want me to open a PR before the work on RQ is merged.

@arikfr
Copy link
Member Author

arikfr commented Oct 15, 2019

This is exciting! :)

When I was running the tests locally I noticed that there were many warnings. Most seemed to be deprecation warnings. Some of them were due to issues in our code and some were due to older dependencies using some other dependencies in a deprecated way (mostly SQLA uses).

Considering we will put the system to a thorough testing due to the Python 3 migration, how about we upgrade our dependencies now that all tests are passing?

You can open the PR regardless, but we do need to decide whether we want to:

  1. Merge Python 3 changes and then rebase the RQ branch.
  2. Merge RQ changes and then rebase the Python 3 branch.

@rauchy wdyt?

@rauchy
Copy link
Contributor

rauchy commented Oct 15, 2019

@arikfr I tend to lean towards #2, mostly because I don't think it's a complicated rebase, but also because I wouldn't want to postpone RQ from production much longer. I can take care of the rebase.

@arikfr
Copy link
Member Author

arikfr commented Oct 15, 2019

Ok, let's aim to merge it soon then. If any problem arises when syncing the Python 3 brancn with these changes, will try to assist.

@NicolasLM
Copy link
Contributor

NicolasLM commented Oct 16, 2019

I saw that the RQ branch was merged, so I rebased my work on master, fixed a few things and pushed a new branch python-3-rq. Everything seems to work well.

@rauchy
Copy link
Contributor

rauchy commented Oct 16, 2019

@NicolasLM awesome, thank you!

@arikfr
Copy link
Member Author

arikfr commented Oct 16, 2019

@NicolasLM super!

Any thoughts on the dependencies update? We can do it in two steps:

  1. Merge Python 3 changes into master.
  2. Update dependencies.

And after that put the whole thing into rigorous testing.

@NicolasLM
Copy link
Contributor

Sounds like a good plan.

@arikfr
Copy link
Member Author

arikfr commented Oct 16, 2019

Let's do it :-)

@jezdez
Copy link
Member

jezdez commented Oct 17, 2019

OMG! I stepped away for a bit and now this! 🎉

@funkindy
Copy link

funkindy commented Nov 20, 2019

@NicolasLM i've just came across a little issue in python query runner. There are couple of py2 builtins (reduce, cmp, unicode) used here:

safe_builtins = (

that breaks the process here:
for key in self.safe_builtins:

would be great if you include this in v3 migration.

@arikfr
Copy link
Member Author

arikfr commented Nov 27, 2019

@funkindy #4375.

@arikfr
Copy link
Member Author

arikfr commented Jan 31, 2020

The Python 3 migration is over. We been running this in production for the past month and overall things look stable.

@arikfr arikfr closed this as completed Jan 31, 2020
@arikfr arikfr unpinned this issue Jan 31, 2020
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

5 participants