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

feat: upgrade docker image to py38 and add support for py39 #16889

Merged
merged 4 commits into from
Oct 5, 2021

Conversation

villebro
Copy link
Member

@villebro villebro commented Sep 29, 2021

SUMMARY

With Python 3.10 right around the corner (2021-10-04) and multiple performance improvements when going from 3.7 to 3.8 (see Python Speed Center benchmark summary below), we should move to 3.8 and start preparing for transitioning to 3.9. This PR bumps the official image from Python 3.7 to 3.8 and adds support for Python 3.9. CI is also bumped from 3.7 to 3.8 (btw, tox was already confgured with basepython = python3.8), and dual tests for 3.7 and 3.8 are replaced with 3.8 and 3.9.

Performance benchmark

On most benchmark tests, 3.8 (blue) performs slightly better than 3.7 (orange):
screencapture-speed-python-org-comparison-2021-09-29-10_32_18

TESTING INSTRUCTIONS

Test locally that everything works as expected

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

sqlalchemy-utils==0.36.8
sqlalchemy-utils==0.37.8
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sqlalchemy-utils==0.36.8 has been yanked from PyPI, causing an ugly warning when installing deps. In addition, py39 was added to the officially supported versions right after 0.37.8 was cut (kvesteri/sqlalchemy-utils@b5b48b1), so I assume 0.37.8 to be fully supported by 3.9 in practice.

pillow>=7.0.0,<8.0.0
pillow>=8.3.1,<9
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pillow 7.x caused problems on Python 3.9

@villebro villebro force-pushed the villebro/upgrade-python branch from 9ecbb2d to 6dea8d0 Compare September 29, 2021 07:32
@villebro villebro changed the title deps: upgrade docker image to py38 and add support for py39 feat: upgrade docker image to py38 and add support for py39 Sep 29, 2021
@villebro villebro force-pushed the villebro/upgrade-python branch 2 times, most recently from 1476881 to b417b90 Compare September 29, 2021 07:42
@villebro villebro force-pushed the villebro/upgrade-python branch from b417b90 to f0583ff Compare September 29, 2021 10:24
@codecov
Copy link

codecov bot commented Sep 29, 2021

Codecov Report

Merging #16889 (38d3b1b) into master (9a8911f) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #16889      +/-   ##
==========================================
+ Coverage   76.85%   76.93%   +0.08%     
==========================================
  Files        1030     1030              
  Lines       55017    55017              
  Branches     7464     7464              
==========================================
+ Hits        42282    42330      +48     
+ Misses      12482    12434      -48     
  Partials      253      253              
Flag Coverage Δ
hive 81.44% <100.00%> (+<0.01%) ⬆️
javascript 70.92% <ø> (ø)
mysql 81.89% <100.00%> (+0.06%) ⬆️
postgres 81.91% <100.00%> (+0.01%) ⬆️
presto 81.77% <100.00%> (?)
python 82.41% <100.00%> (+0.16%) ⬆️
sqlite 81.58% <100.00%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/utils/memoized.py 86.84% <100.00%> (+0.73%) ⬆️
superset/views/database/views.py 88.41% <0.00%> (-0.05%) ⬇️
superset/reports/commands/execute.py 91.22% <0.00%> (+0.38%) ⬆️
superset/models/core.py 90.00% <0.00%> (+0.73%) ⬆️
superset/connectors/sqla/models.py 87.24% <0.00%> (+1.41%) ⬆️
superset/commands/importers/v1/utils.py 91.30% <0.00%> (+2.17%) ⬆️
superset/db_engines/hive.py 85.18% <0.00%> (+3.04%) ⬆️
superset/reports/commands/log_prune.py 89.28% <0.00%> (+3.57%) ⬆️
superset/db_engine_specs/presto.py 90.37% <0.00%> (+6.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a8911f...38d3b1b. Read the comment docs.

Comment on lines -67 to +69
return functools.partial(self.__call__, obj)
func = functools.partial(self.__call__, obj)
func.__func__ = self.func # type: ignore
return func
Copy link
Member Author

@villebro villebro Sep 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't quite able to figure out why this is necessary, but running doctests on memoized methods started failing on 3.9 due to the doctests failing to find the __func__ attribute on memoized methods. The only thing I found is this which seems like the same problem, but is said to affect Python 3.8, not 3.9: https://bugs.python.org/issue12790 Hacky workaround, but does the trick. I'll try to figure out how to solve this more elegantly before this gets merged

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

weird!

Copy link
Member

@amitmiran137 amitmiran137 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Minor change is needed

@@ -168,5 +168,6 @@ def get_git_sha() -> str:
classifiers=[
"Programming Language :: Python :: 3.7",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Programming Language :: Python :: 3.7",

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're still going to support 3.7 (we support last 3 python versions officially). Once 3.10 is out, we'll remove 3.7 support in a major version bump

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to deprecate 3.7 too, but I think we should give it some more time (it's still currently the recommended version) - let's leave it alive for a month or so so everyone can get upgraded to at least 3.8

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@etr2460 @villebro do we—Apache Superset—have an official policy about supporting the last three Python versions? Just asking. If that's the case I think this is great as it means we're not unnecessarily burdened by older less feature rich versions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@john-bodley @etr2460 I think we just lightly agreed on the last 3 versions on a community meeting. Making it an official policy is a good idea to let users know what to expect and plan ahead

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@john-bodley TBH until this PR we were only really supporting two 😄 But yeah, I think there was an unwritten agreement to try to support 3. I'm fine cutting it down to "at least 2" but trying to support 3 if possible.

@villebro
Copy link
Member Author

Opened a task on Apache Infra to bump required tests: https://issues.apache.org/jira/browse/INFRA-22377

@villebro villebro merged commit 82601ab into apache:master Oct 5, 2021
@villebro villebro deleted the villebro/upgrade-python branch October 5, 2021 08:37
@villebro villebro added the v1.4 label Oct 12, 2021
eschutho pushed a commit to preset-io/superset that referenced this pull request Oct 27, 2021
…6889)

* feat: upgrade docker image to py38 and add support for py39

* update required tests

(cherry picked from commit 82601ab)
opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021
…6889)

* feat: upgrade docker image to py38 and add support for py39

* update required tests
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021
…6889)

* feat: upgrade docker image to py38 and add support for py39

* update required tests
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io size/M v1.4 🍒 1.4.0 🍒 1.4.1 🍒 1.4.2 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants