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.

Sign up for GitHub

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

Jump to bottom

Upgrade snowflake-sqlalchemy package #1185

Merged
merged 8 commits into from
Nov 3, 2022
Merged

Upgrade snowflake-sqlalchemy package #1185

merged 8 commits into from
Nov 3, 2022

Conversation

utkarsharma2
Copy link
Collaborator

@utkarsharma2 utkarsharma2 commented Nov 2, 2022 •
edited
Loading

Description

What is the current behavior?

Due to changes in sqlalchemy==1.4.42 our CI is failing, since snowflake-sqlalchemy's rfc imports are failing and we cannot change the version of snowflake-sqlalchemy to anything other than 1.2.4 because airflow 2.2.5 has a constraint.

What is the new behavior?

Upgraded the package to 1.4.3 where the fix is added.

Does this introduce a breaking change?

Nope

@utkarsharma2 utkarsharma2 changed the title Trying fix for CI - optionalPackages Upgrade snowflake-sqlalchemy package Nov 2, 2022
@utkarsharma2 utkarsharma2 mentioned this pull request Nov 2, 2022
Merged
@codecov
Copy link

codecov bot commented Nov 2, 2022 •
edited
Loading

Codecov Report

Base: 94.17% // Head: 94.17% // No change to project coverage 👍

Coverage data is based on head (3f9b271) compared to base (97fc810).
Patch has no changes to coverable lines.

❗ Current head 3f9b271 differs from pull request most recent head 6083221. Consider uploading reports for the commit 6083221 to get more accurate results

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1185   +/-   ##
=======================================
  Coverage   94.17%   94.17%           
=======================================
  Files          13       13           
  Lines         498      498           
  Branches       50       50           
=======================================
  Hits          469      469           
  Misses         20       20           
  Partials        9        9

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

pankajastro
pankajastro reviewed Nov 2, 2022
View reviewed changes
python-sdk/pyproject.toml Outdated
@@ -59,7 +59,7 @@ google = [
]
snowflake = [
"apache-airflow-providers-snowflake",
"snowflake-sqlalchemy>=1.2.0,<=1.2.4",
"snowflake-sqlalchemy>=1.4.3,<1.5.0",
Copy link
Contributor

@pankajastro pankajastro Nov 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

does it cause any problems if we increase the version from 1.2.0 to 1.4.3 and a customer has some other things which depend on 1.2.0 in the env?

@utkarsharma2 utkarsharma2 marked this pull request as draft November 2, 2022 13:38
@utkarsharma2 utkarsharma2 marked this pull request as ready for review November 2, 2022 14:28
@utkarsharma2 utkarsharma2 marked this pull request as draft November 2, 2022 14:30
pankajastro
pankajastro reviewed Nov 2, 2022
View reviewed changes
python-sdk/pyproject.toml Outdated
@@ -23,7 +23,7 @@ dependencies = [
"pyarrow",
"python-frontmatter",
"smart-open",
"SQLAlchemy>=1.3.18"
"SQLAlchemy>=1.3.18,<1.4.42"
Copy link
Contributor

Choose a reason for hiding this comment

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

why we are restricting maybe a comment would be helpful

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have it as part of the PR description, but no harm in adding in the code as well, will do.

@utkarsharma2 utkarsharma2 marked this pull request as ready for review November 2, 2022 14:58
feluelle
feluelle reviewed Nov 2, 2022
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.

Should we have a ticket to unpin this? Do we need the rfc imports?

@utkarsharma2
Copy link
Collaborator Author

@feluelle

  1. We should add the ticket.
  2. So we don't directly use rfc imports, snowflake-sqlalchemy uses it and we cannot change the version of snowflake-sqlalchemy to anything other than 1.2.4 because airflow 2.2.5 has a constraint.

@feluelle
Copy link
Member

feluelle commented Nov 2, 2022 •
edited
Loading

Thanks for the explanation. Could you add the 2nd statement to the PR description, please? And to the comment :)

@pankajastro pankajastro mentioned this pull request Nov 2, 2022
Closed
@utkarsharma2 utkarsharma2 merged commit 2c1c093 into main Nov 3, 2022
@utkarsharma2 utkarsharma2 deleted the FixCISnowflakeSQLA branch November 3, 2022 08:08
@utkarsharma2
Copy link
Collaborator Author

@feluelle Added it in the PR and comment

utkarsharma2 added a commit that referenced this pull request Nov 4, 2022
@utkarsharma2
Upgrade snowflake-sqlalchemy package (#1185)
82e0e85 
# Description
## What is the current behavior?
Due to
[changes](snowflakedb/snowflake-sqlalchemy#350)
in `sqlalchemy==1.4.42` our CI is failing, since snowflake-sqlalchemy's
rfc imports are failing and we cannot change the version of
snowflake-sqlalchemy to anything other than 1.2.4 because airflow 2.2.5
has a constraint.
## What is the new behavior?
Upgraded the package to 1.4.3 where the fix is added.

## Does this introduce a breaking change?
Nope
@feluelle feluelle mentioned this pull request Nov 9, 2022
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feluelle feluelle feluelle left review comments

@pankajastro pankajastro pankajastro approved these changes

@dimberman dimberman Awaiting requested review from dimberman dimberman is a code owner

@tatiana tatiana Awaiting requested review from tatiana tatiana is a code owner

@sunank200 sunank200 Awaiting requested review from sunank200 sunank200 is a code owner

@pankajkoti pankajkoti Awaiting requested review from pankajkoti

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@utkarsharma2 @feluelle @pankajastro