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

chore: remove python2 #310

Merged
merged 4 commits into from
Aug 10, 2020
Merged

chore: remove python2 #310

merged 4 commits into from
Aug 10, 2020

Conversation

dorianj
Copy link
Contributor

@dorianj dorianj commented Aug 6, 2020

Summary of Changes

Remove all direct usage of six (must stay in requirements because other packages still use it), remove python 2 CI.

Ref: amundsen-io/amundsen#560

Tests

Removed Python 2 from CI suite, but no tests modified.

Documentation

README updated to reflect new requirements.

CheckList

Make sure you have checked all steps below to ensure a timely review.

  • PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
  • PR includes a summary of changes.
  • PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
  • PR passes make test

cc @feng-tao

@dorianj dorianj changed the title Remove python2 chore: remove python2 Aug 6, 2020
@codecov-commenter
Copy link

codecov-commenter commented Aug 6, 2020

Codecov Report

Merging #310 into master will decrease coverage by 0.11%.
The diff coverage is 87.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #310      +/-   ##
==========================================
- Coverage   74.30%   74.19%   -0.12%     
==========================================
  Files         105      105              
  Lines        4492     4449      -43     
  Branches      419      405      -14     
==========================================
- Hits         3338     3301      -37     
+ Misses       1049     1041       -8     
- Partials      105      107       +2     
Impacted Files Coverage Δ
databuilder/extractor/db2_metadata_extractor.py 0.00% <0.00%> (ø)
databuilder/extractor/mysql_metadata_extractor.py 0.00% <0.00%> (ø)
...abuilder/extractor/snowflake_metadata_extractor.py 95.00% <ø> (-0.32%) ⬇️
databuilder/callback/call_back.py 92.00% <100.00%> (-0.60%) ⬇️
databuilder/extractor/mssql_metadata_extractor.py 95.23% <100.00%> (-0.29%) ⬇️
...tabuilder/extractor/postgres_metadata_extractor.py 94.82% <100.00%> (-0.34%) ⬇️
databuilder/loader/file_system_neo4j_csv_loader.py 88.46% <100.00%> (-0.83%) ⬇️
databuilder/models/neo4j_csv_serde.py 80.00% <100.00%> (-0.86%) ⬇️
databuilder/models/table_metadata.py 91.78% <100.00%> (-0.04%) ⬇️
databuilder/publisher/neo4j_csv_publisher.py 84.21% <100.00%> (-0.23%) ⬇️
... and 6 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 d24cba9...044eeb3. Read the comment docs.

@dorianj dorianj mentioned this pull request Aug 7, 2020
5 tasks
Copy link
Member

@feng-tao feng-tao left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @dorianj , Also I think we should bump the dep to indicate we remove py2. In terms of typing, I would suggest waiting for a bit as a few soon to be reviewed PR is still open (e.g https://github.com/lyft/amundsendatabuilder/pull/303/files). If we change the mypy rule, it may cause some churns for the open pr. WDYT?

@jfhbrook
Copy link

jfhbrook commented Aug 7, 2020

Out of curiosity: how urgent is this change? I know it's 2020 and all, but I, uh, have use cases for potentially running a custom data builder on top of python 2 code. I totally understand if six is a maintenance burden, but wanted to ask anyway.

@feng-tao
Copy link
Member

feng-tao commented Aug 8, 2020

@jfhbrook, all the prevision pypi should support both py2 and py3.

This change removes python2 support. Python 3.6 or up are now required.

There are no other breaking changes in this release.
@dorianj
Copy link
Contributor Author

dorianj commented Aug 10, 2020

@jfhbrook the biggest improvement is in the ability to upgrade mypy and enforce type annotations. After doing that, I found dozens of legitimate logic errors already in the code (and hundreds of harmless type violations)... so the more we wait, the harder it will be do to get that upgraded

@feng-tao I bumped version from 2.6.5 -> 3.0.0. I believe by semvar this is correct because this is an API breaking change. But happy to do 2.7.0 if you disagree, not sure what norm here is.

With regards to holding the typings PR, I'd rather get it landed, rather than wait. There will be more PRs opened during the time it's being reviewed, which could cause the typing PR to get delayed indefinitely (and require continuous follow-up work). I will personally rebase and add typings to the existing PRs that are out there to make it seamless. wdyt?

@jfhbrook
Copy link

jfhbrook commented Aug 10, 2020

@dorianj yeah I have a lot of sympathy for that. Personally not stoked about maintaining a py2 codebase, just more trying to plan accordingly.

@feng-tao feng-tao merged commit 381a417 into amundsen-io:master Aug 10, 2020
@dorianj
Copy link
Contributor Author

dorianj commented Aug 10, 2020

Thanks Tao!

@dorianj dorianj deleted the dj-remove-py2 branch August 10, 2020 17:20
jerryzhu2007 pushed a commit to kylg/amundsendatabuilder that referenced this pull request Aug 20, 2020
* commit 'e14b33e776929f8b020f1c6fec75d0fb83687693': (23 commits)
  Fix Athena sample DAG (amundsen-io#341)
  fix: Update postgres_sample_dag to set table extract job as upstream for elastic search publisher (amundsen-io#340)
  chore: mypy cleanup (convert last comment types, remove noqa imports) (amundsen-io#338)
  chore: Convert typings to mypy (amundsen-io#311)
  chore: replace all references of Lyft repo with Amundsen (amundsen-io#323)
  feat: add github actions for databuilder (amundsen-io#336)
  build: fix broken tests in Python 3.7, test in CI (amundsen-io#334)
  fix(deps): Unpin attrs (amundsen-io#332)
  ci: add dependabot config (amundsen-io#330)
  Change repo name in travis file (amundsen-io#324)
  tests: add mock for bigquery auth (amundsen-io#313)
  feat: allow hive sql to be provided as config (amundsen-io#312)
  chore: remove python2 (amundsen-io#310)
  chore: update deps for databuilder (amundsen-io#309)
  fix: cypher statement param issue in Neo4jStalenessRemovalTask (amundsen-io#307)
  fix: Added missing job tag key in hive_sample_dag.py (amundsen-io#308)
  feat: enhance glue extractor (amundsen-io#306)
  fix: Fix sql for missing columns and mysql based dialects (#550) (amundsen-io#305)
  docs: Fix broken doc link to dashboard_execution model (amundsen-io#296)
  chore: apply license headers to all the source files (amundsen-io#304)
  ...

# Conflicts:
#	README.md
#	databuilder/extractor/kafka_source_extractor.py
#	databuilder/publisher/neo4j_csv_publisher.py
#	docs/models.md
#	example/scripts/sample_data_loader.py
#	setup.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants