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

[BUGFIX] fix sqlalchemy import #9872

Merged
merged 3 commits into from
May 6, 2024
Merged

Conversation

joshua-stauffer
Copy link
Member

Due to a recent change, convert_to_json_serializable errors if sqlalchemy is not installed.

Copy link

netlify bot commented May 2, 2024

Deploy Preview for niobium-lead-7998 canceled.

Name Link
🔨 Latest commit d0242b9
🔍 Latest deploy log https://app.netlify.com/sites/niobium-lead-7998/deploys/6635899277f03b00089230bd

Copy link

codecov bot commented May 2, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 77.99%. Comparing base (3aaa855) to head (d0242b9).

Files Patch % Lines
great_expectations/core/util.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #9872      +/-   ##
===========================================
- Coverage    77.99%   77.99%   -0.01%     
===========================================
  Files          495      495              
  Lines        42537    42539       +2     
===========================================
+ Hits         33176    33177       +1     
- Misses        9361     9362       +1     
Flag Coverage Δ
3.10 64.21% <66.66%> (-0.01%) ⬇️
3.10 athena or clickhouse or openpyxl or pyarrow or project or sqlite or aws_creds ?
3.10 aws_deps ?
3.10 big ?
3.10 databricks ?
3.10 filesystem ?
3.10 mssql ?
3.10 mysql ?
3.10 postgresql ?
3.10 snowflake ?
3.10 spark ?
3.10 trino ?
3.11 64.21% <66.66%> (-0.01%) ⬇️
3.11 athena or clickhouse or openpyxl or pyarrow or project or sqlite or aws_creds 53.86% <66.66%> (-0.01%) ⬇️
3.11 aws_deps 44.78% <66.66%> (+<0.01%) ⬆️
3.11 big 55.74% <66.66%> (-0.01%) ⬇️
3.11 databricks 45.96% <66.66%> (+<0.01%) ⬆️
3.11 filesystem 61.16% <66.66%> (-0.01%) ⬇️
3.11 mssql 48.77% <33.33%> (+<0.01%) ⬆️
3.11 mysql 48.83% <66.66%> (+<0.01%) ⬆️
3.11 postgresql 52.74% <66.66%> (-0.01%) ⬇️
3.11 snowflake 46.57% <66.66%> (+<0.01%) ⬆️
3.11 spark 57.16% <66.66%> (-0.01%) ⬇️
3.11 trino 50.74% <66.66%> (-0.01%) ⬇️
3.8 64.22% <66.66%> (-0.02%) ⬇️
3.8 athena or clickhouse or openpyxl or pyarrow or project or sqlite or aws_creds 53.86% <66.66%> (-0.01%) ⬇️
3.8 aws_deps 44.80% <66.66%> (+<0.01%) ⬆️
3.8 big 55.75% <66.66%> (-0.01%) ⬇️
3.8 databricks 45.97% <66.66%> (+<0.01%) ⬆️
3.8 filesystem 61.18% <66.66%> (-0.01%) ⬇️
3.8 mssql 48.75% <33.33%> (+<0.01%) ⬆️
3.8 mysql 48.81% <66.66%> (+<0.01%) ⬆️
3.8 postgresql 52.73% <66.66%> (-0.01%) ⬇️
3.8 snowflake 46.58% <66.66%> (+<0.01%) ⬆️
3.8 spark 57.13% <66.66%> (-0.01%) ⬇️
3.8 trino 50.72% <66.66%> (-0.01%) ⬇️
3.9 64.23% <66.66%> (-0.01%) ⬇️
3.9 athena or clickhouse or openpyxl or pyarrow or project or sqlite or aws_creds ?
3.9 aws_deps ?
3.9 big ?
3.9 databricks ?
3.9 filesystem ?
3.9 mssql ?
3.9 mysql ?
3.9 postgresql ?
3.9 snowflake ?
3.9 spark ?
3.9 trino ?
cloud 0.00% <0.00%> (ø)
docs-basic 49.10% <66.66%> (+<0.01%) ⬆️
docs-creds-needed 50.22% <66.66%> (-0.01%) ⬇️
docs-spark 48.31% <66.66%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@joshua-stauffer joshua-stauffer changed the title [MAINTENANCE] fix sqlalchemy import [BUGFIX] fix sqlalchemy import May 2, 2024
@@ -64,6 +64,9 @@
if not LegacyRow:
LegacyRow = SQLALCHEMY_NOT_IMPORTED

if not Row:
Copy link
Contributor

@billdirks billdirks May 2, 2024

Choose a reason for hiding this comment

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

This and the above LegacyRow conditional should not be necessary, the compatibility file already does this . SQLALCHEMY_NOT_IMPORTED evaluates to False so this should just be overwriting Row with the same thing.

Could you remove this and try with just your change below? You could also verify this here by checking if Row is already equal to SQLALCHEMY_NOT_IMPORTED

Copy link
Member Author

Choose a reason for hiding this comment

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

that makes sense to me, but unfortunately it isn't the case. If we remove those null checks SQLALCHEMY_NOT_IMPORTED raises a ModuleNotFoundError because something is calling __getattr__ on SQLALCHEMY_NOT_IMPORTED. Seems like something isn't quite working as expected, but not clear what, so i'll defer looking further and go with the established pattern.

@joshua-stauffer joshua-stauffer added this pull request to the merge queue May 6, 2024
Merged via the queue into develop with commit d455f54 May 6, 2024
68 checks passed
@joshua-stauffer joshua-stauffer deleted the m/v1/fix_sqlalchemy_error branch May 6, 2024 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants