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

[AIRFLOW-4296][AIP-3-subtask-18] Remove py2 in ci process #5090

Merged
merged 1 commit into from
Apr 20, 2019

Conversation

zhongjiajie
Copy link
Member

Make sure you have checked all steps below.

Jira

Description

My PR #5020 CI failed, and the sample log https://api.travis-ci.org/v3/job/519213992/log.txt

Traceback (most recent call last):
  File "/usr/local/bin/airflow", line 21, in <module>
    from airflow import configuration
  File "/usr/local/lib/python2.7/dist-packages/airflow/__init__.py", line 36, in <module>
    from airflow import settings, configuration as conf
  File "/usr/local/lib/python2.7/dist-packages/airflow/settings.py", line 32, in <module>
    from airflow.utils.sqlalchemy import setup_event_handlers
  File "/usr/local/lib/python2.7/dist-packages/airflow/utils/sqlalchemy.py", line 22, in <module>
    import json
  File "/usr/local/lib/python2.7/dist-packages/airflow/utils/json.py", line 27, in <module>
    class AirflowJsonEncoder(json.JSONEncoder):
AttributeError: 'module' object has no attribute 'JSONEncoder'

And I found out we ci process still have py2, So remove from __future__ import absolute_import will cause error. This PR are try to fix this.

Code Quality

  • Passes flake8

@zhongjiajie
Copy link
Member Author

@Fokko @ashb @XD-DENG PTAL.

@kaxil
Copy link
Member

kaxil commented Apr 12, 2019

This is already done and merged in master:

3020d97

@zhongjiajie
Copy link
Member Author

@kaxil I rebase on master but PR-5019 and PR-5020 still failed.

@Fokko
Copy link
Contributor

Fokko commented Apr 14, 2019

Restarted the CI

@zhongjiajie
Copy link
Member Author

Still failed, I try to fix it.

@@ -19,7 +19,7 @@

set -x

cd /usr/local/lib/python2.7/dist-packages/airflow && \
cd /usr/local/lib/python3.6/dist-packages/airflow && \
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this path should use python3.5 instead of python3.6. Xenial LTS installs python3.5 unless we add a new PPA.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for catching. But I change base docker images to python:3.6-slim because we have a jira ticket in https://issues.apache.org/jira/browse/AIRFLOW-4210. And I think we should change our ci k8s as well.

@zhongjiajie zhongjiajie force-pushed the ci_remove_py2_content branch 2 times, most recently from 59aa927 to a4097bb Compare April 15, 2019 06:28
@zhongjiajie
Copy link
Member Author

Add gcc because of failed in https://travis-ci.org/apache/airflow/jobs/520123725

@zhongjiajie zhongjiajie force-pushed the ci_remove_py2_content branch from a4097bb to 9ccf73f Compare April 15, 2019 08:16
@codecov-io
Copy link

Codecov Report

Merging #5090 into master will decrease coverage by 0.47%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5090      +/-   ##
==========================================
- Coverage   77.23%   76.76%   -0.48%     
==========================================
  Files         465      465              
  Lines       29837    29837              
==========================================
- Hits        23046    22905     -141     
- Misses       6791     6932     +141
Impacted Files Coverage Δ
airflow/contrib/kubernetes/volume_mount.py 33.33% <0%> (-66.67%) ⬇️
airflow/contrib/kubernetes/pod_launcher.py 38.21% <0%> (-52.04%) ⬇️
airflow/contrib/kubernetes/volume.py 50% <0%> (-50%) ⬇️
airflow/contrib/kubernetes/pod_generator.py 43.24% <0%> (-43.25%) ⬇️
...rflow/contrib/operators/kubernetes_pod_operator.py 56.16% <0%> (-42.47%) ⬇️
airflow/contrib/kubernetes/kube_client.py 27.58% <0%> (-37.94%) ⬇️
airflow/configuration.py 85.95% <0%> (-4.8%) ⬇️
airflow/models/taskinstance.py 92.59% <0%> (+0.17%) ⬆️

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 1c6aa5e...9ccf73f. Read the comment docs.

1 similar comment
@codecov-io
Copy link

codecov-io commented Apr 15, 2019

Codecov Report

Merging #5090 into master will decrease coverage by 0.47%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5090      +/-   ##
==========================================
- Coverage   77.23%   76.76%   -0.48%     
==========================================
  Files         465      465              
  Lines       29837    29837              
==========================================
- Hits        23046    22905     -141     
- Misses       6791     6932     +141
Impacted Files Coverage Δ
airflow/contrib/kubernetes/volume_mount.py 33.33% <0%> (-66.67%) ⬇️
airflow/contrib/kubernetes/pod_launcher.py 38.21% <0%> (-52.04%) ⬇️
airflow/contrib/kubernetes/volume.py 50% <0%> (-50%) ⬇️
airflow/contrib/kubernetes/pod_generator.py 43.24% <0%> (-43.25%) ⬇️
...rflow/contrib/operators/kubernetes_pod_operator.py 56.16% <0%> (-42.47%) ⬇️
airflow/contrib/kubernetes/kube_client.py 27.58% <0%> (-37.94%) ⬇️
airflow/configuration.py 85.95% <0%> (-4.8%) ⬇️
airflow/models/taskinstance.py 92.59% <0%> (+0.17%) ⬆️

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 1c6aa5e...9ccf73f. Read the comment docs.

@zhongjiajie
Copy link
Member Author

zhongjiajie commented Apr 15, 2019

CI green. But before merge I'd like to test if this change work in my personal repo https://github.com/zhongjiajie/airflow/tree/test_if_ci_work

Travis CI log is here https://travis-ci.com/zhongjiajie/airflow/builds/108322089

I use this PR and cherry-pick #5020. submit https://github.com/zhongjiajie/airflow/tree/test_if_ci_work

@Fokko
Copy link
Contributor

Fokko commented Apr 15, 2019

@zhongjiajie Still a sad CI :'( Can you rerun the job?

@zhongjiajie
Copy link
Member Author

Seem failed with un-related test https://travis-ci.com/zhongjiajie/airflow/jobs/192913602
But I will try again to ensure.

@zhongjiajie
Copy link
Member Author

zhongjiajie commented Apr 15, 2019

https://travis-ci.com/zhongjiajie/airflow/builds/108333920

K8S ci pass, but postgre failed and I think the failed is un-related

======================================================================
46) ERROR: test_execution_limited_parallelism (tests.executors.test_local_executor.LocalExecutorTest)
----------------------------------------------------------------------
   Traceback (most recent call last):
    tests/executors/test_local_executor.py line 75 in test_execution_limited_parallelism
      self.execution_parallelism(parallelism=test_parallelism)
    tests/executors/test_local_executor.py line 61 in execution_parallelism
      self.assertTrue(executor.event_buffer['fail'], State.FAILED)
   KeyError: 'fail'

And this postgre job sucess in https://travis-ci.com/zhongjiajie/airflow/builds/108322089

Should I restart the CI in my repo or just accept/review this PR
WDYT @Fokko

@milton0825
Copy link
Contributor

@zhongjiajie can you restart the tests in your CI? Probably just a flaky test.

@zhongjiajie
Copy link
Member Author

@milton0825 I think is flaky test, but in job https://travis-ci.com/zhongjiajie/airflow/builds/108333920 we could found out K8S test job pass(we just want to prove K8S test pass). So I wonder if we could accept this PR

@zhongjiajie
Copy link
Member Author

@milton0825
Copy link
Contributor

lgtm

@jmcarp
Copy link
Contributor

jmcarp commented Apr 19, 2019

Anything left to do on this? It looks good to me.

@mik-laj
Copy link
Member

mik-laj commented Apr 19, 2019

PTAL @potiuk

@milton0825
Copy link
Contributor

Yeah let's ship this as it blocks the PY2 deprecation.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Looks good. Hopefully I got to fix the last issue with multi-staging docker soon and we get rid of it altogether ;)

@zhongjiajie
Copy link
Member Author

zhongjiajie commented Apr 20, 2019

Ping @Fokko @ashb @kaxil @XD-DENG PTAL.

@zhongjiajie
Copy link
Member Author

@potiuk Do you have the rights to merge PR now. I check my dev-email and notice you become committer. BTW Congratulations 👍

@potiuk potiuk merged commit 0ac501f into apache:master Apr 20, 2019
@zhongjiajie zhongjiajie deleted the ci_remove_py2_content branch April 21, 2019 03:56
@zhongjiajie
Copy link
Member Author

@potiuk Thanks for merging.

@zhongjiajie
Copy link
Member Author

zhongjiajie commented Apr 21, 2019

@potiuk Notice that you don't change jira ticket status as RESOLVED, maybe you should take a look as #4966 (comment). FYI

@potiuk
Copy link
Member

potiuk commented Apr 21, 2019

Yeah..I realised I have no necessary rights in JIRA yet. I am waiting for infra team to grant me the right. But I keep the list of tickets I merged so no worries !

Happy Easter :)

@Fokko
Copy link
Contributor

Fokko commented Apr 21, 2019

Thanks @potiuk for picking this up. Happy Easter as well! 👍

andriisoldatenko pushed a commit to andriisoldatenko/airflow that referenced this pull request Jul 26, 2019
wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019
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.

8 participants