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

fix(tests): Remove walrus operator for Python 3.7 compatiblity #18205

Merged
merged 1 commit into from
Jan 28, 2022

Conversation

ad-m
Copy link
Contributor

@ad-m ad-m commented Jan 27, 2022

SUMMARY

A new feature called assignment expressions has been added to Python v3.8. A new syntactical operator— “The Walrus Operator” is part of it. The syntax of it is not backward compatible.

The minimum Python version supported by Apache Superset is 3.7, so we can not use any v3.8+ non-backward compatible features of Python.

superset/setup.py

Lines 172 to 176 in 568b8e1

classifiers=[
"Programming Language :: Python :: 3.7",
"Programming Language :: Python :: 3.8",
"Programming Language :: Python :: 3.9",
],

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TESTING INSTRUCTIONS

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

That code was introduced 9 days ago on master via #18060. PR was created by @ofekisr and accepted by @amitmiran137. Could you take a look here?

@codecov
Copy link

codecov bot commented Jan 27, 2022

Codecov Report

Merging #18205 (7795a06) into master (568b8e1) will decrease coverage by 0.18%.
The diff coverage is n/a.

❗ Current head 7795a06 differs from pull request most recent head 5e4857b. Consider uploading reports for the commit 5e4857b to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18205      +/-   ##
==========================================
- Coverage   66.05%   65.86%   -0.19%     
==========================================
  Files        1591     1591              
  Lines       62418    62418              
  Branches     6286     6286              
==========================================
- Hits        41228    41113     -115     
- Misses      19568    19683     +115     
  Partials     1622     1622              
Flag Coverage Δ
hive 52.17% <ø> (ø)
mysql ?
postgres ?
presto 52.01% <ø> (ø)
python 81.39% <ø> (-0.38%) ⬇️
sqlite 81.03% <ø> (ø)

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

Impacted Files Coverage Δ
superset/databases/commands/create.py 31.37% <0.00%> (-56.87%) ⬇️
superset/sql_validators/postgres.py 50.00% <0.00%> (-50.00%) ⬇️
superset/views/database/mixins.py 60.34% <0.00%> (-20.69%) ⬇️
superset/databases/commands/update.py 85.71% <0.00%> (-8.17%) ⬇️
superset/common/utils/dataframe_utils.py 85.71% <0.00%> (-7.15%) ⬇️
superset/databases/api.py 87.10% <0.00%> (-5.93%) ⬇️
superset/databases/dao.py 94.44% <0.00%> (-5.56%) ⬇️
superset/views/database/validators.py 78.94% <0.00%> (-5.27%) ⬇️
superset/databases/schemas.py 94.27% <0.00%> (-4.20%) ⬇️
superset/db_engine_specs/mysql.py 93.97% <0.00%> (-3.62%) ⬇️
... and 12 more

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 568b8e1...5e4857b. Read the comment docs.

@ofekisr
Copy link
Contributor

ofekisr commented Jan 27, 2022

@ad-m
But superset does not support 3.7 anymore and you don't have test here in the Ci to check it
@amitmiran137

@ad-m
Copy link
Contributor Author

ad-m commented Jan 27, 2022

@ofekisr when Superset dropped support for Python 3.7? I see more indication about support Python 3.7 compatibility i codebase eg.:

PYTHON=`command -v python3.9 || command -v python3.8 || command -v python3.7`

if ! [ -x "${PYTHON}" ]; then echo "You need Python 3.7, 3.8 or 3.9 installed"; exit 1; fi

Discussion in #16889 mentions switching CI to Python 3.8 (mostly), but keeping support for the last 3 versions of Python. Python v3.10 is not supported yet.

@amitmiran137
Copy link
Member

What is your take on that @villebro ? I thought we switch to 3.8 and 3.9

@ad-m ad-m closed this Jan 27, 2022
@ad-m ad-m reopened this Jan 27, 2022
@ad-m
Copy link
Contributor Author

ad-m commented Jan 27, 2022

Thank you for presenting these doubts. This looks python versions supported are a project-wide issue that goes far beyond the scope of this PR of a few line changes.

I have the impression that comments under pull requests on GitHub are not a permanent and long-term solution to achieve a consensus and a clear policy in this regard. A mature project such as Apache Superset must have a clear policy of supported Python versions to provide users with a secure plan for the necessary updates.

I will open discussions on this on the dev@ list so that all interested parties can share their views known so that we can reach a real consensus instead of relying on an "unwritten agreement".

@villebro
Copy link
Member

@amitmiran137 @ofekisr as long as setup.py lists 3.7 as supported, I can't really see how we can introduce the walrus operator. Having said that, we did raise deprecation of Python 3.7 many months ago on the dev list(link), so maybe we should just declare 3.7 deprecated now. @ad-m is there a reason why you wish to extend 3.7 support, or are you ok moving to 3.8?

@ad-m
Copy link
Contributor Author

ad-m commented Jan 27, 2022

@ad-m is there a reason why you wish to extend 3.7 support, or are you ok moving to 3.8?

I am ok to move to 3.8. I use Python 3.7, because that's how I setup the project as a supported Python version. I have noticed a strange behavior of the project after rebase (see my comment at https://github.com/apache/superset/pull/17530/files#r793833184 also), which indicates that code incompatible with Python 3.7 is leaking into this project, which has unexpected effects for me.

I started gathering facts for a mailing list message... In SIP-57 we indicated that breaking changes mean: "Any changes that are version bumps in Python that would require users to upgrade their systems. We currently support the last three minor versions of Python.". The same document gives guidelines about announcing deprecations to users: "The changelog should contain information about features that are deprecated, and clearly notate when it is going to be permanently removed.".

I am unable to find in UPDATING.md any information about deprecation or breaking support for Python 3.7, only about Python 2 deprecated in Superset 0.28.0. Could we add some notes about the announcement there also? I am new to the project and haven't gone through the full history of the discussion on the mailing list.

In my opinion, the user contract resulting from semantic versioning (mainly the values for the entire community) requires us to fix support for Python 3.7 (I will be happy to help with this in terms of my knowledge of the project), then we released the patch version, which restores Python 3.7 support for end-users (I do not know if 1.4.0 and earlier has real support for Python 3.7) and carried out the Python 3.7 deprecation process as defined in SIP-57. In this case, Python 3.7 could be dropped sometime after Python 3.10 support was added to maintain support for the three Python releases as claimed in SIP-57.

@ad-m ad-m closed this Jan 27, 2022
@ad-m ad-m reopened this Jan 27, 2022
@villebro
Copy link
Member

Merging this to unblock Python 3.7 support. I will follow-up with a PR to officially deprecate support for 3.7 as per the announcement on the dev list.

@villebro villebro merged commit 84db896 into apache:master Jan 28, 2022
shcoderAlex pushed a commit to casual-precision/superset that referenced this pull request Feb 7, 2022
ofekisr pushed a commit to nielsen-oss/superset that referenced this pull request Feb 8, 2022
bwang221 pushed a commit to casual-precision/superset that referenced this pull request Feb 10, 2022
@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 size/XS 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants