Page MenuHomePhabricator

Upgrade thumbor to Thumbor 7 and python3
Closed, ResolvedPublic

Description

Thumbor 7 will remove support for Python 2, requiring Python 3.6+. Previous versions of Thumbor do not support Python 3. That means that Debian Buster is required (T216815).

There are a number of breaking changes in Thumbor 7 (https://github.com/thumbor/thumbor/releases/tag/7.0.0a5), mostly around replacing callbacks with async/await.

Thumbor 7 will also allow extensions to easily add new handlers, which may obsolete some of our custom code.

Migration plan of porting the project to Python 3.7:

  1. The following items must be checked more precisely. If they exist in the thumbor-plugins project, they must be definitely fixed before the migartion:
    1. using // instead of / when dividing integers
    2. using new-style classes
    3. separating binary data and strings
    4. using the iterator-methods on dictionaries
    5. incorrect imports
    6. relative imports problem
    7. indentation
    8. rounding behavior
    9. reorganizations and renamings
    10. binding method
  2. We decided to use 2to3 fixer, since it exists in Python 3.7 version. So the second step is to run the fixer and make the code Python3 compatible.
  3. The last step is to update all the dependencies to necessary versions which are compatible with Python 3.7 and rewrite the code according to new features of the dependencies if it is necessary. We will also need to update the setup.py file to denote Python 3 compatibility.

Event Timeline

This work will also require us moving to a supported version of py3exiv2 - as far as I can see the one we currently use from stretch is of the 0.x family which is no longer available in pip. I am not certain how incompatible this version will be but in testing I have already seen some breakage when using the 1.x family.

In Wikimedia code, pyexiv2 is only used to read image orientation, exiftool is used for everything else. Thumbor proper uses py3exiv2, but might switch to pyexiv2 (which has its own problems and probably wouldn't be suitable in Wikimedia production).

roman-stolar updated the task description. (Show Details)
roman-stolar updated the task description. (Show Details)
roman-stolar subscribed.

Change 800170 had a related patch set uploaded (by Vlad.shapik; author: Vlad.shapik):

[operations/software/thumbor-plugins@master] WP:Upgrade thumbor to Thumbor 7 and python3

https://gerrit.wikimedia.org/r/800170

Change 810862 had a related patch set uploaded (by Hashar; author: Hashar):

[integration/config@master] dockerfiles: add libexiv2-dev to tox-buster

https://gerrit.wikimedia.org/r/810862

Change 810862 merged by jenkins-bot:

[integration/config@master] dockerfiles: add libexiv2-dev to tox-buster

https://gerrit.wikimedia.org/r/810862

Change 810866 had a related patch set uploaded (by Hashar; author: Hashar):

[integration/config@master] jjb: update jobs for tox-buster

https://gerrit.wikimedia.org/r/810866

Change 810866 merged by jenkins-bot:

[integration/config@master] jjb: update jobs for tox-buster

https://gerrit.wikimedia.org/r/810866

Change 810870 had a related patch set uploaded (by Hashar; author: Hashar):

[integration/config@master] dockerfiles: add libboost-python-dev to tox-buster

https://gerrit.wikimedia.org/r/810870

Change 810870 merged by Hashar:

[integration/config@master] dockerfiles: add libboost-python-dev to tox-buster

https://gerrit.wikimedia.org/r/810870

Change 810871 had a related patch set uploaded (by Hashar; author: Hashar):

[integration/config@master] jjb: update jobs for tox-buster

https://gerrit.wikimedia.org/r/810871

Change 810871 merged by jenkins-bot:

[integration/config@master] jjb: update jobs for tox-buster

https://gerrit.wikimedia.org/r/810871

Change 800170 merged by jenkins-bot:

[operations/software/thumbor-plugins@master] Upgrade thumbor to Thumbor 7 and python3

https://gerrit.wikimedia.org/r/800170

Since we want to migrate Wikimedia Thumbor to the new version of Python 3 & Thumbor 7 we decided to move the desired functionality from Thumbor Community Core to Wikimedia Thumbor, as currently, the Thumbor Community Core don't support new versions of Python 3 and Thumbor 7 […]

Was there communication attempted with upstream? And/or our changes provided as pull request?

Perhaps this already happened elsewhere, but I've informed upstream at https://github.com/thumbor-community/core/issues/21, thus continuing the conversation that started with Gilles in the year before at https://github.com/thumbor-community/core/issues/20.

Hi @Krinkle , when we are working on migration Wikimedia Thumbor to Python 3.7 and Thumbor 7, we have a discussion and decide to fork required code from Thumbor Community Core to reduce the number of dependencies that need to be maintained.

Hi, I'm trying to understand https://gerrit.wikimedia.org/r/c/operations/software/thumbor-plugins/+/800170/ a bit better.

  • What parts of this change are coming from upstream and what parts are new?
  • How did https://gerrit.wikimedia.org/r/c/operations/software/thumbor-plugins/+/489022/6 ended up getting included, when it has not (AFAICT) been merged?
  • Did the new code get thoroughly reviewed? I only looked at one file, tests/integration/test_swift.py, and it looks like the change made at least some of the test code unreachable -- adding assert False to mock_get_object() does not result in a test failure.

@Vlad.shapik, @roman-stolar, ping on the above :)
I'd also like to understand the deployment plan for this. Are you working with anyone on Wikimedia SRE to get this deployed? I strongly recommend deploying small, incremental updates rather than accumulating a lot of changes.

Hi @ori!
We are working with @hnowlan, who is responsible to get this deployed.
We fork "core" code source from Thumbor Community Core to Wikimedia Thumbor to reduce the number of dependencies that need to be maintained. (At the moment TC Core didn't support Python 3.7 and Thumbor 7).
We look into repository that already has some useful things for migration as it was configure for Debian Buster and Python 3+.
Also after migration we update packages and code sources to be relevant with new version of Debian Buster, Python 3.7 and Thumbor 7.

Thanks @roman-stolar. I think it's a mistake to combine (a) changes from (multiple?) upstream(s), (b) unmerged changes from Gerrit, and (c) your own work into a single commit, as in Icabc39dab. This destroys some useful history (for example, the authorship, review comments and discussion on Id6ec6d62c), and it makes future reconciliation with upstream code harder. It's also error-prone.

I suggest reverting the patch and re-applying these changes one by one, with proper attribution and testing.

@Vlad.shapik thank you, but what about the other points I raised?

Would it be possible for you to create a new branch for the migration? This way we would still have a way to roll out smaller changes to production.

Hi @ori , we plan to discuss this case on meeting tomorrow with @hnowlan.

Maybe I describe in a wrong way on my previous message, sorry for that. All of the changes on this patch was related to migration and couldn't be split to a patches. Only a changes related to Gilles patch was additionally added to migration patch and it was our mistake/misunderstanding, so we will revert it as @Vlad.shapik wrote above.

Also we have a patch/branch with stable/old version (Python 2.7, Debian 9) where "thumbor community core" used as a dependency.

Change 825788 had a related patch set uploaded (by Vlad.shapik; author: Vlad.shapik):

[operations/software/thumbor-plugins@master] Revert setting expiry headers on thumbnails

https://gerrit.wikimedia.org/r/825788

Change 825788 merged by jenkins-bot:

[operations/software/thumbor-plugins@master] Revert setting expiry headers on thumbnails

https://gerrit.wikimedia.org/r/825788

Hi, I'm trying to understand https://gerrit.wikimedia.org/r/c/operations/software/thumbor-plugins/+/800170/ a bit better.

  • What parts of this change are coming from upstream and what parts are new?
  • How did https://gerrit.wikimedia.org/r/c/operations/software/thumbor-plugins/+/489022/6 ended up getting included, when it has not (AFAICT) been merged?
  • Did the new code get thoroughly reviewed? I only looked at one file, tests/integration/test_swift.py, and it looks like the change made at least some of the test code unreachable -- adding assert False to mock_get_object() does not result in a test failure.

Hi @ori
FYI: I have filed the issue about not triggering mock_get_object() in test_swift.py. I have checked that it is even on the Dockerized Python2 version of thumbor-plugins. So, it looks like the migration to Python3 didn't cause it.