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

move pymssql back to an optional requirement #4581

Merged
merged 6 commits into from
Jan 31, 2024
Merged

Conversation

adamsachs
Copy link
Contributor

@adamsachs adamsachs commented Jan 30, 2024

Closes PROD-1558

Description Of Changes

Pull out the pymssql driver dependency into an optional requirements file that is not installed as part of our python package install by default. This is because the driver does not include functionality that's essential for our python package functionality, and it's known to cause problems and require workarounds on M1 macs. The optional dependency can still be installed as part of the python package by including an [all] extras keyword in the package dependency reference, e.g. ethyca-fides[all]==2.28.1a3.

The pymssql dependency (and any others we decide to put in optional-requirements.txt) will still be installed as part of all of our docker image builds. It functions fine on linux containers, and it's important functionality for our deployed application (which is a different use case than our python package!)

Code Changes

  • make an optional-requirements.txt to separate out optional requirements not needed for basic python package/CLI usage
  • ensure the optional-requirements are installed on our docker image builds via the Dockerfile, since they're needed for our images and there are no problem with these libraries on linux containers
  • allow optional requirements to be specified as an "extra" with the python package install, either via the requirement name mssql or via an all keyword, e.g. pip install ethyca-fides[all]==2.28.1a3

Steps to Confirm

  • confirmed locally that i could install the ethyca-fides python package (and therefore run nox -s "fides_env(test)" even with freetds uninstalled and no cached pymssql packages, i.e. our python package install and the fides_env command do not require installing pymssql nor building its wheel on the host machine, while the package is installed and wheel is built on the docker image that's built by fides deploy up. the following steps used to produce a build error on an M1 Mac locally, but now no longer do with this update:

    • uninstall freetds locally: brew uninstall --ignore-dependencies freetds
    • remove cached wheels from global pip cache: find ~/Library/Caches/pip/wheels/ | grep pymssql to find and then remove
    • remove pymssql packages from the repo's local nox caches (./.nox), e.g. from the repo root:rm -rf .nox/fides_env-slim/lib/python3.10/site-packages/pymssql*
    • test out running nox -s "fides_env(test)". on main, this should fail on an M1 mac when it tries to build the pymssql wheel. but on this branch, things should succeed
  • mssql integration tests still seem to be running well in CI, since that's using the built docker image that has the package installed 👍 https://github.com/ethyca/fides/actions/runs/7727125996/job/21065508406?pr=4581

Pre-Merge Checklist

Copy link

vercel bot commented Jan 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
fides-plus-nightly ⬜️ Ignored (Inspect) Visit Preview Jan 31, 2024 4:50pm

Copy link

cypress bot commented Jan 30, 2024

Passing run #6125 ↗︎

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

Details:

Merge bf4ba37 into acc2541...
Project: fides Commit: f9de01f47a ℹ️
Status: Passed Duration: 00:39 💡
Started: Jan 31, 2024 3:10 PM Ended: Jan 31, 2024 3:10 PM

Review all test suite changes for PR #4581 ↗︎

Copy link

codecov bot commented Jan 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (acc2541) 86.81% compared to head (bf4ba37) 86.81%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4581   +/-   ##
=======================================
  Coverage   86.81%   86.81%           
=======================================
  Files         330      330           
  Lines       19839    19839           
  Branches     2545     2545           
=======================================
  Hits        17223    17223           
  Misses       2150     2150           
  Partials      466      466           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@adamsachs adamsachs added the run unsafe ci checks Runs fides-related CI checks that require sensitive credentials label Jan 31, 2024
@adamsachs adamsachs self-assigned this Jan 31, 2024
README.md Outdated Show resolved Hide resolved
optional-requirements.txt Outdated Show resolved Hide resolved
@adamsachs adamsachs marked this pull request as ready for review January 31, 2024 15:00
Copy link
Contributor

@NevilleS NevilleS left a comment

Choose a reason for hiding this comment

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

This looks good to me, I pushed some changes to the README that I think work!

optional-requirements.txt Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@adamsachs adamsachs merged commit 6ee23bb into main Jan 31, 2024
18 checks passed
@adamsachs adamsachs deleted the asachs/PROD-1558 branch January 31, 2024 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run unsafe ci checks Runs fides-related CI checks that require sensitive credentials
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants