Skip to content

[AIRFLOW-5748] Remove python auto-detection#6423

Merged
potiuk merged 1 commit intoapache:masterfrom
PolideaInternal:remove-python-autodetection
Oct 25, 2019
Merged

[AIRFLOW-5748] Remove python auto-detection#6423
potiuk merged 1 commit intoapache:masterfrom
PolideaInternal:remove-python-autodetection

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Oct 24, 2019

Make sure you have checked all steps below.

Jira

Description

  • Here are some details about my PR, including screenshots of any UI changes:

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • 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
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

ashb
ashb reviewed Oct 24, 2019
View reviewed changes
common/_autodetect_variables.sh Outdated
Copy link
Member

@ashb ashb Oct 24, 2019

Choose a reason for hiding this comment

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

Suggested change
# Unless python_version is
# Unless python_version is set

Half-written comment?

feluelle
feluelle approved these changes Oct 24, 2019
View reviewed changes
Copy link
Member

@feluelle feluelle left a comment

Choose a reason for hiding this comment

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

LGTM, I read it in slack but would be nice if you could add the reason why you want to remove it to the commit and/or PR description.

kaxil
kaxil reviewed Oct 24, 2019
View reviewed changes
common/_autodetect_variables.sh Outdated
Copy link
Member

@kaxil kaxil Oct 24, 2019

Choose a reason for hiding this comment

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

Suggested change
export BASE_IMAGE_NAME=${IMAGE_NAME:=${DOCKERHUB_USER}/${DOCKERHUB_REPO}:${AIRFLOW_CONTAINER_BRANCH_NAME}-python${PYTHON_VERSION=3.5}}
export BASE_IMAGE_NAME=${IMAGE_NAME:=${DOCKERHUB_USER}/${DOCKERHUB_REPO}:${AIRFLOW_CONTAINER_BRANCH_NAME}-python${PYTHON_VERSION:=3.5}}

??

Copy link
Member Author

@potiuk potiuk Oct 24, 2019

Choose a reason for hiding this comment

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

Yeah. Good catch - but it was supposed to be :- :)

Copy link
Member Author

@potiuk potiuk Oct 24, 2019

Choose a reason for hiding this comment

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

I do not want to set PYTHON_VERSION in this case - just use 3.5 if not defined

@potiuk potiuk force-pushed the remove-python-autodetection branch 2 times, most recently from d7f9885 to d030b6d Compare October 24, 2019 22:59
@potiuk
Copy link
Member Author

potiuk commented Oct 24, 2019

LGTM, I read it in slack but would be nice if you could add the reason why you want to remove it to the commit and/or PR description.

Done in the commit message.

@feluelle
Copy link
Member

feluelle commented Oct 24, 2019

I like that 👍

@potiuk
[AIRFLOW-5748] Remove python auto-detection
6e632db 
The detection of python version is complex because we need to handle
several cases - including determining the version from image name
on DockerHub, detecting python version from python in the environment,
finally forced from python version. This caused multiple problems
with Travis where we run tests with different version (auto-detected
from current python - especially when python3 became present in
Travis' python 2.7 images. Now all the jobs in Travis have
PYTHON_VERSION forced and the code responsible for detecting current
python version has been removed as it is not needed in this case.
@potiuk potiuk force-pushed the remove-python-autodetection branch from d030b6d to 6e632db Compare October 25, 2019 06:05
@potiuk potiuk merged commit 351ae4e into apache:master Oct 25, 2019
vladimir-gavrilenko added a commit to vladimir-gavrilenko/airflow that referenced this pull request Oct 25, 2019
@vladimir-gavrilenko
[AIRFLOW-5748] Remove python auto-detection (apache#6423)
74be280 
The detection of python version is complex because we need to handle
several cases - including determining the version from image name
on DockerHub, detecting python version from python in the environment,
finally forced from python version. This caused multiple problems
with Travis where we run tests with different version (auto-detected
from current python - especially when python3 became present in
Travis' python 2.7 images. Now all the jobs in Travis have
PYTHON_VERSION forced and the code responsible for detecting current
python version has been removed as it is not needed in this case.
ashb pushed a commit to ashb/airflow that referenced this pull request Oct 25, 2019
@potiuk @ashb
[AIRFLOW-5748] Remove python auto-detection (apache#6423)
d601752 
The detection of python version is complex because we need to handle
several cases - including determining the version from image name
on DockerHub, detecting python version from python in the environment,
finally forced from python version. This caused multiple problems
with Travis where we run tests with different version (auto-detected
from current python - especially when python3 became present in
Travis' python 2.7 images. Now all the jobs in Travis have
PYTHON_VERSION forced and the code responsible for detecting current
python version has been removed as it is not needed in this case.

(cherry picked from commit 351ae4e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kaxil kaxil kaxil left review comments

@ashb ashb ashb approved these changes

@feluelle feluelle feluelle approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@potiuk @feluelle @ashb @kaxil