Replace pyodbc with pymssql #3435

ThomasLaPiana · 2023-06-02T05:55:57Z

Closes #3152

Note to reviewers: as I developed this on an x86 platform, I need help confirming from ARM users that everything works as expected!

Code Changes

add to requirements.txt
remove the dangerous-requirements.txt
update the Dockerfile to remove the platform-checking magic
update documentation
remove the logic from setup.py
fix how MS SQL test data is seeded

Steps to Confirm

on an ARM chip (Mac M-series), the nox -s build command works as expected (doesn't use emulation, chooses the right platform automatically)
on an ARM chip, connecting to an MSSQL server instance with pymssql works (can try running the ops integration tests locally)

Pre-Merge Checklist

All CI Pipelines Succeeded
Documentation:
- documentation complete, updated in our dev docs
Issue Requirements are Met
~~Relevant Follow-Up Issues Created~~
Update CHANGELOG.md

Description Of Changes

This is an investigation into and a possible solution for replacing pyodbc with pymssql as our SQL Server adapter. It doesn't come with the same platform caveats as pyodbc and therefore would simplify and streamline our build/deploy logic.

This PR fully removes some of the platform-specific "magic" we've been doing, and simplifies the build session as well as the Dockerfile. We're no longer locked out of mssql connections on ARM chips

cypress · 2023-06-02T06:12:27Z

Passing run #2743 ↗︎

0	4	0	0	0
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.

Details:

Merge `5f28b54` into `45c0bca`...
Project: fides	Commit: `cacbb82ba6 ℹ️`
Status: Passed	Duration: 00:52 💡
Started: Jun 16, 2023 6:50 AM	Ended: Jun 16, 2023 6:51 AM

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

codecov · 2023-06-02T06:31:42Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.11 🎉

Comparison is base (ba4f5e4) 87.01% compared to head (9b9adf5) 87.13%.

❗ Current head 9b9adf5 differs from pull request most recent head 057db90. Consider uploading reports for the commit 057db90 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3435      +/-   ##
==========================================
+ Coverage   87.01%   87.13%   +0.11%     
==========================================
  Files         311      312       +1     
  Lines       18863    18762     -101     
  Branches     2402     2390      -12     
==========================================
- Hits        16414    16348      -66     
+ Misses       2025     1991      -34     
+ Partials      424      423       -1

Impacted Files	Coverage Δ
...nnection_configuration/connection_secrets_mssql.py	`100.00% <ø> (ø)`
src/fides/api/service/connectors/sql_connector.py	`74.77% <ø> (+7.61%)`	⬆️
src/fides/core/utils.py	`88.33% <100.00%> (+0.09%)`	⬆️

... and 43 files with indirect coverage changes

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

ThomasLaPiana · 2023-06-02T09:40:06Z

I've narrowed the issue down to autocommit. Our test setup relies on this as it creates/drops databases but it works slightly differently with pymssql. It'll take some fenagling but there is definitely a way forward here

ThomasLaPiana · 2023-06-02T15:43:02Z

@SteveDMurphy I'm getting this very close....

SteveDMurphy · 2023-06-02T19:42:28Z

@SteveDMurphy I'm getting this very close....

Excited to see you jumped into this !! More pressing on this has come recently from @mfbrown too 👍🏽

How do you feel about the idea of switching these after getting your hands dirty a bit?? I see both are still listed as supported by Microsoft, and the recent activity on the repo is nice but I'm a little concerned the recent commits haven't generated a release candidate just yet 🤔

ThomasLaPiana · 2023-06-03T02:14:36Z

@SteveDMurphy I'm getting this very close....

Excited to see you jumped into this !! More pressing on this has come recently from @mfbrown too 👍🏽

How do you feel about the idea of switching these after getting your hands dirty a bit?? I see both are still listed as supported by Microsoft, and the recent activity on the repo is nice but I'm a little concerned the recent commits haven't generated a release candidate just yet 🤔

@SteveDMurphy as in, how do I feel about making the switch from pyodbc to pymssql after working on this PR? My feeling is that this is vastly superior. The only reasons I've run into any problem at all was because the connectors are slightly different for writing to the database, but for reading they're identical.

This completely mitigates the issues we saw here, as well as completely removes the need for the dangerous-requirements.txt which also means we can remove some of the platform logic from the docker commands.

Overall this one change leads to significant simplification across the whole codebase. Although there haven't been recent commits, database drivers in general should be slow-moving as they are a standard and shouldn't need to be updated much. As long as it allows us to read from SQL Server databases, which hopefully should "never" change, I think we're fine here.

ThomasLaPiana · 2023-06-04T06:49:07Z

final two test failures appear to be caused by the new driver returning a different error...going to diagnose but everything else looks like its working

ThomasLaPiana · 2023-06-04T15:24:45Z

@SteveDMurphy something else I want to test/confirm here is that with these changes, we no longer need the platform logic in the docker_nox.py file...we should now be able to let Docker figure that out automatically since we don't have a step that potentially breaks things and requires a certain platform...

I'm going to make that change and attempt to test it myself by forcing the build platform to arm64

ThomasLaPiana · 2023-06-05T04:39:04Z

I'm hitting some bugs locally when trying to build as an arm64 platform, seems to specifically be around openssl. Adding apt dependencies to see if I can fix it...

Dockerfile

noxfiles/docker_nox.py

tests/ops/integration_tests/test_connection_configuration_integration.py

adamsachs · 2023-06-05T20:10:40Z

@ThomasLaPiana nox -s build succeeded and seemed to work well for me, so that hurdle is gone. however i was still getting errors connecting to mssql when running nox -s "pytest(ops-integration) such that those integration tests are failing for me. here's an example stack trace:

connection_config_mssql = <fides.api.models.connectionconfig.ConnectionConfig object at 0xffff8173b4c0>

    @pytest.fixture(scope="function")
    def mssql_integration_session_cls(connection_config_mssql):
        uri = MicrosoftSQLServerConnector(connection_config_mssql).build_uri()
>       engine = get_db_engine(database_uri=uri)

tests/fixtures/mssql_fixtures.py:66:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
/usr/local/lib/python3.10/site-packages/fides/api/db/session.py:34: in get_db_engine
    return create_engine(
/root/.local/lib/python3.10/site-packages/sqlalchemy/util/deprecations.py:309: in warned
    return fn(*args, **kwargs)
/root/.local/lib/python3.10/site-packages/sqlalchemy/engine/create.py:560: in create_engine
    dbapi = dialect_cls.dbapi(**dbapi_args)
/root/.local/lib/python3.10/site-packages/sqlalchemy/dialects/mssql/pymssql.py:81: in dbapi
    module = __import__("pymssql")
/root/.local/lib/python3.10/site-packages/pymssql/__init__.py:3: in <module>
    from ._pymssql import *
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

>   ???
E   ImportError: libsybdb.so.5: cannot open shared object file: No such file or directory

src/pymssql/_pymssql.pyx:1: ImportError

haven't had a whole lot of time to look into it further but posting it now just to give an incremental update. curious what others with an M1 are seeing.

SteveDMurphy · 2023-06-05T23:07:34Z

haven't had a whole lot of time to look into it further but posting it now just to give an incremental update. curious what others with an M1 are seeing.

Same errors from me, still haven't gotten in too deep but wanted to second what @adamsachs is seeing so far 👍🏽

ThomasLaPiana · 2023-06-06T05:42:09Z

aha: pymssql/pymssql#533

ThomasLaPiana · 2023-06-07T03:21:25Z

@adamsachs @SteveDMurphy can y'all try again? I added some more deps that might fix it...it's basically dependency whackamole at this point I think

adamsachs · 2023-06-07T13:01:56Z

unfortunately still seem to be getting similar errors...

src/fides/api/service/connectors/base_connector.py:60: in client
    self.db_client = self.create_client()
src/fides/api/service/connectors/sql_connector.py:169: in create_client
    return create_engine(
<string>:2: in create_engine
    ???
/opt/fides/lib/python3.10/site-packages/sqlalchemy/util/deprecations.py:309: in warned
    return fn(*args, **kwargs)
/opt/fides/lib/python3.10/site-packages/sqlalchemy/engine/create.py:560: in create_engine
    dbapi = dialect_cls.dbapi(**dbapi_args)
/opt/fides/lib/python3.10/site-packages/sqlalchemy/dialects/mssql/pymssql.py:81: in dbapi
    module = __import__("pymssql")
/opt/fides/lib/python3.10/site-packages/pymssql/__init__.py:3: in <module>
    from ._pymssql import *
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

>   ???
E   ImportError: libsybdb.so.5: cannot open shared object file: No such file or directory

src/pymssql/_pymssql.pyx:1: ImportError

ThomasLaPiana · 2023-06-07T13:32:46Z

unfortunately still seem to be getting similar errors...

src/fides/api/service/connectors/base_connector.py:60: in client
    self.db_client = self.create_client()
src/fides/api/service/connectors/sql_connector.py:169: in create_client
    return create_engine(
<string>:2: in create_engine
    ???
/opt/fides/lib/python3.10/site-packages/sqlalchemy/util/deprecations.py:309: in warned
    return fn(*args, **kwargs)
/opt/fides/lib/python3.10/site-packages/sqlalchemy/engine/create.py:560: in create_engine
    dbapi = dialect_cls.dbapi(**dbapi_args)
/opt/fides/lib/python3.10/site-packages/sqlalchemy/dialects/mssql/pymssql.py:81: in dbapi
    module = __import__("pymssql")
/opt/fides/lib/python3.10/site-packages/pymssql/__init__.py:3: in <module>
    from ._pymssql import *
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

>   ???
E   ImportError: libsybdb.so.5: cannot open shared object file: No such file or directory

src/pymssql/_pymssql.pyx:1: ImportError

Hmmm dang. Ok, the feedback loop is a little too slow to solve this efficiently, let's try to grab some time and pair on this sync so we can get this finalized or close it

Kelsey-Ethyca · 2023-06-08T16:48:07Z

before merging this can we investigate the longevity of pymssql because it might be deprecated soon?

ThomasLaPiana · 2023-06-09T08:11:15Z

before merging this can we investigate the longevity of pymssql because it might be deprecated soon?

Where does it look like it will be deprecated soon? Or do you mean abandoned?

I just looked at the repo and mailing list, and while it isn't the most active project it isn't really "dead" in my opinion?

Kelsey-Ethyca · 2023-06-13T05:30:28Z

I was wrong. In 2019 it was stated that pymssql would be discontinued but that was obviously reversed and active today as you stated. I was kind of hoping we could get some guarantee of continued support. Don't let me hold you up any longer pymssql/pymssql#668

SteveDMurphy

Working locally for me, 🤞🏽 that checks pass as well 🤞🏽

ThomasLaPiana · 2023-06-16T05:46:21Z

it looks like the merge super broke stuff 😢

Update: figured it out, new migration went into the old folder, moved it to the new one

Replace pyodbc with pymssql

75a5197

ThomasLaPiana self-assigned this Jun 2, 2023

feat: replace all instances of pyodbc with pymssql

5cb32f6

ThomasLaPiana added the run unsafe ci checks Runs fides-related CI checks that require sensitive credentials label Jun 2, 2023

ThomasLaPiana added 3 commits June 2, 2023 15:02

docs: changelog

31102d5

fix: docker health check

0f53c9c

feat: more progress on fixing the MSSQL tests

1ca4066

ThomasLaPiana added 3 commits June 2, 2023 22:40

fix: update how mssql is seeded in tests

d84c139

feat: update external database setup patterns

cf92429

fix: mssql server testing for ctl

147dc1b

ThomasLaPiana added 3 commits June 3, 2023 10:21

fix: run all mssql queries in a single connection

554eb6d

Merge branch 'main' into ThomasLaPiana-replace-pyodbc-pymssql

5bc32a2

fix: remove the hardcoded driver from the mssql connector

eea9046

ThomasLaPiana added 3 commits June 5, 2023 15:14

feat: add dependencies to allow for building on ARM

e3ff6c6

fix: connection tests

56bedff

fix: pylint

c8fe8c6

ThomasLaPiana requested review from SteveDMurphy, adamsachs and TheAndrewJackson June 5, 2023 08:07

ThomasLaPiana marked this pull request as ready for review June 5, 2023 11:43

ThomasLaPiana commented Jun 5, 2023

View reviewed changes

Dockerfile Show resolved Hide resolved

ThomasLaPiana commented Jun 5, 2023

View reviewed changes

noxfiles/docker_nox.py Show resolved Hide resolved

ThomasLaPiana commented Jun 5, 2023

View reviewed changes

tests/ops/integration_tests/test_connection_configuration_integration.py Show resolved Hide resolved

fix: add other deps

ec1d5c3

Merge branch 'main' into ThomasLaPiana-replace-pyodbc-pymssql

9b9adf5

SteveDMurphy and others added 3 commits June 15, 2023 16:58

fix: include freetds in backend build stage

b30462d

Merge branch 'main' into ThomasLaPiana-replace-pyodbc-pymssql

7951f00

fix: static checks

057db90

SteveDMurphy approved these changes Jun 15, 2023

View reviewed changes

ThomasLaPiana added 3 commits June 16, 2023 10:52

Merge branch 'main' into ThomasLaPiana-replace-pyodbc-pymssql

eb30d53

fix: merged changes, static checks

7fa3948

fix: import

6d09ed2

ThomasLaPiana added 2 commits June 16, 2023 13:59

fix: remove mention of pyodbc

2fc1c34

fix: move the new migration from main into the new migrations folder

5f28b54

ThomasLaPiana merged commit df3a624 into main Jun 16, 2023

ThomasLaPiana deleted the ThomasLaPiana-replace-pyodbc-pymssql branch June 16, 2023 07:28

adamsachs mentioned this pull request Jul 19, 2023

pymssql dependency breaks build for M1/ARM users #3824

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace pyodbc with pymssql #3435

Replace pyodbc with pymssql #3435

ThomasLaPiana commented Jun 2, 2023 •

edited

Loading

cypress bot commented Jun 2, 2023 •

edited

Loading

codecov bot commented Jun 2, 2023 •

edited

Loading

ThomasLaPiana commented Jun 2, 2023

ThomasLaPiana commented Jun 2, 2023

SteveDMurphy commented Jun 2, 2023

ThomasLaPiana commented Jun 3, 2023

ThomasLaPiana commented Jun 4, 2023

ThomasLaPiana commented Jun 4, 2023

ThomasLaPiana commented Jun 5, 2023

adamsachs commented Jun 5, 2023

SteveDMurphy commented Jun 5, 2023

ThomasLaPiana commented Jun 6, 2023

ThomasLaPiana commented Jun 7, 2023

adamsachs commented Jun 7, 2023

ThomasLaPiana commented Jun 7, 2023

Kelsey-Ethyca commented Jun 8, 2023

ThomasLaPiana commented Jun 9, 2023 •

edited

Loading

Kelsey-Ethyca commented Jun 13, 2023

SteveDMurphy left a comment

ThomasLaPiana commented Jun 16, 2023 •

edited

Loading

Replace pyodbc with pymssql #3435

Replace pyodbc with pymssql #3435

Conversation

ThomasLaPiana commented Jun 2, 2023 • edited Loading

Note to reviewers: as I developed this on an x86 platform, I need help confirming from ARM users that everything works as expected!

Code Changes

Steps to Confirm

Pre-Merge Checklist

Description Of Changes

cypress bot commented Jun 2, 2023 • edited Loading

Passing run #2743 ↗︎

codecov bot commented Jun 2, 2023 • edited Loading

Codecov Report

ThomasLaPiana commented Jun 2, 2023

ThomasLaPiana commented Jun 2, 2023

SteveDMurphy commented Jun 2, 2023

ThomasLaPiana commented Jun 3, 2023

ThomasLaPiana commented Jun 4, 2023

ThomasLaPiana commented Jun 4, 2023

ThomasLaPiana commented Jun 5, 2023

adamsachs commented Jun 5, 2023

SteveDMurphy commented Jun 5, 2023

ThomasLaPiana commented Jun 6, 2023

ThomasLaPiana commented Jun 7, 2023

adamsachs commented Jun 7, 2023

ThomasLaPiana commented Jun 7, 2023

Kelsey-Ethyca commented Jun 8, 2023

ThomasLaPiana commented Jun 9, 2023 • edited Loading

Kelsey-Ethyca commented Jun 13, 2023

SteveDMurphy left a comment

Choose a reason for hiding this comment

ThomasLaPiana commented Jun 16, 2023 • edited Loading

ThomasLaPiana commented Jun 2, 2023 •

edited

Loading

cypress bot commented Jun 2, 2023 •

edited

Loading

codecov bot commented Jun 2, 2023 •

edited

Loading

ThomasLaPiana commented Jun 9, 2023 •

edited

Loading

ThomasLaPiana commented Jun 16, 2023 •

edited

Loading