RFC: Upgrade sqlalchemy #5

leo-mansini · 2022-12-29T23:04:05Z

No description provided.

…ade-sqlalchemy

leo-mansini · 2022-12-29T23:07:12Z

muttlib/dbconn/base.py

@@ -126,11 +126,14 @@ def __setattr__(self, attr, value):
                elif attr == 'driver':
                    value = format_drivername(self.dialect, value)
                attr = 'drivername'
+            self.conn_url = self.conn_url.set(**{attr: value})


https://docs.sqlalchemy.org/en/20/changelog/migration_14.html#change-5526

I don't quickly understand the change, mind explaining it to me if you remember? :)

If it's a breaking change with sqlalchemy 1.3 we could add a version check to remain compatible with both 1.3 and 1.4.

Yes, this is a breaking change. Previously one could just do

connection.url = "my_url"

But now the attribute is protected from changes like that, and requires to be changed with its set method.

Regarding double compatibility, I'm answering you in another comment.

This reverts commit 8db59e4.

leo-mansini · 2023-01-04T16:41:01Z

pyproject.toml

 numpy = ">=1.22.0,<2.0"
 jinjasql = "^0.1.8"
-ipython = "^7.29.0"
+ipython = ">=8.0.1"


Motivated not by sqlalchemy but by vulnerability issue.

https://www.cvedetails.com/cve/CVE-2022-21699/

https://ipython.readthedocs.io/en/stable/whatsnew/version8.html#ipython-8-0-1-cve-2022-21699

MartinCura

Hi 👋🏼 @leo-mansini.

Nice PR you've got there. Is this ready for action? Do you want me to spam someone so it's code reviewed? It looks fairly inoffensive, with the caveat that the PR name is not complete as you're modifying several package, aren't you?

I was just looking to upgrade project-lisa to mlflow 1.28.0 which requires sqlalchemy 1.4... but this lovely package muttlib doesn't let me :) (because of the current sqlalchemy <1.4 limitation) ... unless you merge with the change i'm requesting.

MartinCura · 2023-03-27T15:51:15Z

pyproject.toml

@@ -58,28 +58,27 @@ progressbar2 = "^3.55.0"
 PyYAML = ">=5.1,<7.0"
 scikit-learn = "^1.0.1"
 scipy = "^1.7.2"
-SQLAlchemy = "<1.4.0,>=1.3.0"
+SQLAlchemy = "^1.4,<1.5"


Could it be changed to >=1.3,<2.0? This would retain compatibility with many more projects already using muttlib. Or is the 1.3 -> 1.4 upgrade breaking?

<2.0 and <1.5 is the same since there aren't any versions in >=1.5, <2.0, but I think <2.0 is better.

Regarding >=1.3, I can try and check!

However I'm not sure how to manage this with MLFlow or other libraries not compatible with 1.3, would muttlib need compatibility for the older version of those libraries too?

I just started a poetry project with sqlalchemy 1.3 and this branch to test this. In the last commit I attempted the double compatibility and no issues with poetry or pytest 👍 , just need to test it on sqlalchemy 1.3 since poetry.lock set it to 1.4.

MartinCura · 2023-03-27T15:58:17Z

muttlib/dbconn/base.py

@@ -126,11 +126,14 @@ def __setattr__(self, attr, value):
                elif attr == 'driver':
                    value = format_drivername(self.dialect, value)
                attr = 'drivername'
+            self.conn_url = self.conn_url.set(**{attr: value})


I don't quickly understand the change, mind explaining it to me if you remember? :)

If it's a breaking change with sqlalchemy 1.3 we could add a version check to remain compatible with both 1.3 and 1.4.

leo-mansini · 2023-03-27T16:18:52Z

@MartinCura answering this. The changes in dependencies are, as far as I can remember, all motivated by sqlalchemy version bump, except for IPython, where I added the vulnerability comment.

Other cases that I remember were particular are:

sqlalchemy-trino not only had problems with sqlalchemy but is also deprecated as it was added as an extra to Trino.
wrapt was not used, and also had conflicts with sqlalchemy.

MartinCura · 2023-03-27T16:29:35Z

OK great, and what do you think about retaining compatibility for both sqlalchemy 1.3 and 1.4?

MartinCura · 2023-03-27T16:30:09Z

@CrossNox do i bother you for a Code Review? 😅

leo-mansini · 2023-03-27T17:18:02Z

pyproject.toml

 psycopg2-binary = { version = "^2.9.2", optional = true }
 PyMySQL = { version = "^1.0.2", optional = true }
 pymssql = { version = "^2.2.2", optional = true }
 pymongo = { version = "^3.12.1", optional = true }
 snowflake-connector-python = { version = "^2.7.1", optional = true }
 cryptography = { version = "3.4.7", optional = true }
-snowflake-sqlalchemy = { version = "1.2.4", optional = true }
+snowflake-sqlalchemy = { version = ">=1.2.4, <=1.4.4, !=1.2.5", optional = true } # https://github.com/snowflakedb/snowflake-sqlalchemy/issues/234


Avoiding 1.2.5 because of incompatibility with sqlalchemy 1.3 (source).

leo-mansini · 2023-03-27T17:23:13Z

@MartinCura Added compatibility with SQLAlchemy >1.3. Keep in mind the changes were tested only in the project included tests and in my particular uses, which are Google Sheets and BigQuery.

MartinCura · 2023-03-28T11:08:22Z

pyproject.toml

 matplotlib = "^3.5.0"
 pyarrow = { version = "6.0.0", optional = true }
 cx-Oracle = { version = "^8.3.0", optional = true }
 PyHive = { version = ">=0.6.1", optional = true }
-thrift = { version = "^0.15.0", optional = true }
+thrift = { version = "0.16.0", optional = true }


pinning to a specific version for same reason? i.e. missing ^?

question remains

Leaving it with ^ works anyways as latest is 0.16.0 so no reason to not have it 🤷

MartinCura · 2023-03-28T11:09:17Z

@MartinCura Added compatibility with SQLAlchemy >1.3. Keep in mind the changes were tested only in the project included tests and in my particular uses, which are Google Sheets and BigQuery.

Thanks! Looking good!

leo-mansini · 2023-06-09T22:08:34Z

Merged this to master, just have an issue with bandit, fails due to a pd.read_pickle I didn't modify in this PR 😢

leo-mansini

CI passes @aoelvp94 @MartinCura

We can merge if it lgty

leo-mansini · 2023-09-29T18:47:00Z

muttlib/utils.py

@@ -183,7 +183,7 @@ def clean_quotes(s):
    elif ext == 'feather':
        return pd.read_feather(fn)
    elif ext in ['pickle', 'pkl']:
-        return pd.read_pickle(fn)
+        return pd.read_pickle(fn)  # nosec


For bandit to pass it's either this or removing the feature.

maybe add a comment to entrypoint function cautioning about passing unsafe pickle files because it's a security issue; apart from that i can't think of anything else you can do?

Added to docstring (if by entrypoint you mean the df_read_multi util).

and maybe local_df_cache()

muttlib/dbconn/base.py

MartinCura · 2023-09-29T19:18:11Z

muttlib/utils.py

@@ -183,7 +183,7 @@ def clean_quotes(s):
    elif ext == 'feather':
        return pd.read_feather(fn)
    elif ext in ['pickle', 'pkl']:
-        return pd.read_pickle(fn)
+        return pd.read_pickle(fn)  # nosec


maybe add a comment to entrypoint function cautioning about passing unsafe pickle files because it's a security issue; apart from that i can't think of anything else you can do?

MartinCura · 2023-09-29T19:18:56Z

pyproject.toml

 matplotlib = "^3.5.0"
 pyarrow = { version = "6.0.0", optional = true }
 cx-Oracle = { version = "^8.3.0", optional = true }
 PyHive = { version = ">=0.6.1", optional = true }
-thrift = { version = "^0.15.0", optional = true }
+thrift = { version = "0.16.0", optional = true }


question remains

muttlib/utils.py

MartinCura

neat

i'm checking whether there's some project that is endangered because of lack of lockfile, but after that we can merge

MartinCura · 2023-10-10T12:38:41Z

i'm checking whether there's some project that is endangered because of lack of lockfile, but after that we can merge

Seems like there's none :)

leo-mansini · 2023-10-10T13:00:34Z

TY @MartinCura for the review!

CrossNox and others added 25 commits October 6, 2022 02:54

migrate workflows

3cf8de7

fix error?

8c2cdc4

(wip) chore: fix actions syntax errors, multiline scripts, env vars

3c88ea6

(wip) chore: specify python container

e380a90

(wip) chore: specify poetry version, python container for version.yml

0544300

(wip) chore: remove container in favor of setup-python

3eef593

debug(CI): sample workflow

52e8bc3

test action runs on

6e3cf16

Update .github/workflows/test.yml

5e44e3a

Update .github/workflows/test.yml

a496da5

(wip) chore: add container image for run_tests

a1c9dca

(wip) chore: remove sudo for run_tests

8d4c42b

(wip) chore: add container image for all workflows

1829004

(wip) fix: add MIN_LINE_RATE var

fc32a68

(wip) fix: replace gitlab vars with github vars

08bad96

(wip) chore: delete sample workflow

10fb8e7

(wip) add echo for testing purposes

9b6b526

(wip) set id for tag output

2f6874a

(wip) bump version

db448bf

add libraries for sqlalchemy 1.4

7da6ec8

rename numpy object dtype

43a7d42

change ibis import path

7907c18

support immutable sqlalchemy url

7496753

Merge remote-tracking branch 'origin/migration-from-gitlab' into upgr…

2b88c4d

…ade-sqlalchemy

add implicit optionals

d761b8b

leo-mansini changed the base branch from master to migration-from-gitlab December 29, 2022 23:04

leo-mansini commented Dec 29, 2022

View reviewed changes

leo-mansini added 3 commits January 4, 2023 13:37

upgrade ipython

68b99f6

ipython version was 8.0.1

8db59e4

Revert "ipython version was 8.0.1"

4b6b4ea

This reverts commit 8db59e4.

leo-mansini commented Jan 4, 2023

View reviewed changes

Remove outdated wrapt

95d2a30

MartinCura reviewed Mar 27, 2023

View reviewed changes

leo-mansini added 2 commits March 27, 2023 13:48

allow sqlalchemy ver 1.3

6a1782d

avoid snowflake-sqlalchemy 1.2.5

b45e52a

leo-mansini commented Mar 27, 2023

View reviewed changes

isort

9121b11

MartinCura reviewed Mar 28, 2023

View reviewed changes

Base automatically changed from migration-from-gitlab to master April 5, 2023 19:42

leo-mansini added 2 commits June 9, 2023 18:37

Merge branch 'master' into upgrade-sqlalchemy

375728a

bump pyproject.toml version

11f76fb

nosec on read_pickle

da078a2

leo-mansini commented Sep 29, 2023

View reviewed changes

MartinCura reviewed Sep 29, 2023

View reviewed changes

leo-mansini added 2 commits September 29, 2023 16:27

note on pickle vulnerability

2ea3644

support thrift 0.16 and above

a5ce98a

MartinCura reviewed Oct 2, 2023

View reviewed changes

muttlib/utils.py Outdated Show resolved Hide resolved

MartinCura approved these changes Oct 2, 2023

View reviewed changes

further clarification on unpickling docstring

04dbd23

MartinCura assigned leo-mansini Oct 10, 2023

leo-mansini merged commit 753f2a1 into master Oct 10, 2023

MartinCura deleted the upgrade-sqlalchemy branch October 18, 2023 14:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Upgrade sqlalchemy #5

RFC: Upgrade sqlalchemy #5

leo-mansini commented Dec 29, 2022

leo-mansini Dec 29, 2022

MartinCura Mar 27, 2023

leo-mansini Mar 27, 2023

leo-mansini Jan 4, 2023

MartinCura left a comment

MartinCura Mar 27, 2023

leo-mansini Mar 27, 2023

leo-mansini Mar 27, 2023

MartinCura Mar 27, 2023

leo-mansini commented Mar 27, 2023

MartinCura commented Mar 27, 2023

MartinCura commented Mar 27, 2023

leo-mansini Mar 27, 2023

leo-mansini commented Mar 27, 2023 •

edited

Loading

MartinCura Mar 28, 2023

MartinCura Sep 29, 2023

leo-mansini Sep 29, 2023

MartinCura commented Mar 28, 2023

leo-mansini commented Jun 9, 2023

leo-mansini left a comment

leo-mansini Sep 29, 2023

MartinCura Sep 29, 2023

leo-mansini Sep 29, 2023

MartinCura Oct 2, 2023

MartinCura Sep 29, 2023

MartinCura Sep 29, 2023

MartinCura left a comment

MartinCura commented Oct 10, 2023

leo-mansini commented Oct 10, 2023

RFC: Upgrade sqlalchemy #5

RFC: Upgrade sqlalchemy #5

Conversation

leo-mansini commented Dec 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinCura left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leo-mansini commented Mar 27, 2023

MartinCura commented Mar 27, 2023

MartinCura commented Mar 27, 2023

Choose a reason for hiding this comment

leo-mansini commented Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinCura commented Mar 28, 2023

leo-mansini commented Jun 9, 2023

leo-mansini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinCura left a comment

Choose a reason for hiding this comment

MartinCura commented Oct 10, 2023

leo-mansini commented Oct 10, 2023

leo-mansini commented Mar 27, 2023 •

edited

Loading