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

Switches to latest version of snowflake connector #13654

Merged

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Jan 13, 2021

This should allow us to release a new version of snowflake
provider that is not interacting with other providers via
monkeypatching of SSL classes.

Fixes #12881


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

setup.py Show resolved Hide resolved
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.

Nice!

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Jan 13, 2021
@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

Dockerfile Outdated Show resolved Hide resolved
@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@potiuk potiuk force-pushed the switch-to-latest-snowflake-connector branch 3 times, most recently from c42033c to c92417c Compare January 14, 2021 06:24
@potiuk
Copy link
Member Author

potiuk commented Jan 14, 2021

@kaxil @ashb @mik-laj @ephraimbuddy @turbaszek - when fixing the 'azure-storage' problems connected (they will be finaly solved in #12188 which I rebased on top of this one) I also simplified upgrades of setup.py so they will also automatically work in case any change in setup.py results in REMOVING dependency (as is in the case of azure-storage).

Basically - no more EPOCH changes are needed when you remove dependencies. Whenever setup.py/setup.cfg is changed all installation will skip automatically (on CI) the "cache" build and it will install everything from the scratch - that will take much longer than usual (3-4 minutes) but it should handle every case - including the removal.

I did it as part of the 'snowflake' fix because also here I want to reinstall dependencies from scratch.

@potiuk potiuk force-pushed the switch-to-latest-snowflake-connector branch 5 times, most recently from 4b111ad to 9c7348c Compare January 14, 2021 13:06
Dockerfile.ci Outdated
Comment on lines 301 to 305
# are uninstalled, only dependencies remain.
# the cache is only used when "upgrade to newer dependencies" is not set to automatically
# account for removed dependencies (we do not install them in the first place)
RUN if [[ ${AIRFLOW_PRE_CACHED_PIP_PACKAGES} == "true" && \
${UPGRADE_TO_NEWER_DEPENDENCIES} != "true" ]]; then \
Copy link
Member

Choose a reason for hiding this comment

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

This will default to True right? So will use cache and L329 will take care of installing it if setup.py or setup.cfg changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

UPGRADE_TO_NEWER_DEPENDENCIES is "false" by default.

It is set in the 'selective_checks" in CI to "true" (as of few days) whenever setup.py or setup.cfg changes (

upgrade_to_newer_dependencies="${INCOMING_COMMIT_SHA}"
)

This means that in CI, by default the "install_airflow_from_latest_master.sh" is used (and airflow is installed from cache first).

When either of the two setup files change, UPGRADE_TO_LATEST_DEPENDENCIES is set and this line is skipped, so no preinstalled packages - they are all installed from scratch (which will take a bit longer but it is 'clean' state - so anything that disappears (comparing to master) is not installed.

The situation is different when you build image locally - when you change setup.py, setup.cfg and build the image. the cache is still used (UPGRADE_TO_NEWER_DEPENDENCIES) is "false").

This way you avoid rebuilding all of the dependencies when you add new dependency (it takes ~ 10 minutes) to install all deps from the scratch.

You can still trigger the same behavior as in CI by adding --upgrade-to-newer-dependencies flag in breeze when building the image (it simply sets UPGRADE_TO_NEWER_DEPENDENCIES).

However, good that I explained it - I just realised that the comparision should be == "false" because we are using commit_hash as the "truthy" value in selective checks! Fixing it.

@github-actions
Copy link

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

@potiuk potiuk force-pushed the switch-to-latest-snowflake-connector branch from 9c7348c to 748c461 Compare January 14, 2021 13:40
@github-actions
Copy link

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

@potiuk potiuk force-pushed the switch-to-latest-snowflake-connector branch from 748c461 to 1c121ab Compare January 16, 2021 07:45
@potiuk potiuk force-pushed the switch-to-latest-snowflake-connector branch from 1c121ab to b3aa198 Compare January 16, 2021 09:26
This should allow us to release a new version of snowflake
provider that is not interacting with other providers via
monkeypatching of SSL classes.

Fixes apache#12881
@potiuk potiuk force-pushed the switch-to-latest-snowflake-connector branch from b3aa198 to 51a6659 Compare January 16, 2021 10:27
@potiuk potiuk mentioned this pull request Jan 16, 2021
@potiuk
Copy link
Member Author

potiuk commented Jan 16, 2021

Random test failure only. Merging!

@potiuk potiuk merged commit 6e90dfc into apache:master Jan 16, 2021
@potiuk potiuk deleted the switch-to-latest-snowflake-connector branch January 16, 2021 11:52
@kaxil
Copy link
Member

kaxil commented Jan 16, 2021

Should this go on 2.0.1 @potiuk ?

@potiuk
Copy link
Member Author

potiuk commented Jan 16, 2021

Absolutely!

@potiuk potiuk added this to the Airflow 2.0.1 milestone Jan 16, 2021
kaxil pushed a commit that referenced this pull request Jan 21, 2021
This should allow us to release a new version of snowflake
provider that is not interacting with other providers via
monkeypatching of SSL classes.

Fixes #12881

(cherry picked from commit 6e90dfc)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Snowflake python connector monkeypatches urllib and makes many services unusable.
5 participants