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

fix: upgrade python requirements #596

Merged
merged 2 commits into from
Jul 5, 2022
Merged

fix: upgrade python requirements #596

merged 2 commits into from
Jul 5, 2022

Conversation

rebkwok
Copy link
Contributor

@rebkwok rebkwok commented Jul 5, 2022

Fixes #579

Upgrade all prod and dev requirements except sqlalchemy**

This addresses the current security alerts for numpy and pyjwt.

Note that it appears dependabot doesn't currently support pyproject.toml outside of poetry - see this issue, so prod dependencies are not being updated as expected.

** sqlachemy is now pinned to 1.4.27; upgrading past 1.4.27 results in warnings which cause test errors, e.g.:

SAWarning: Dialect mssql:pymssql will not make use of SQL compilation caching as it does not set the 'supports_statement_cache' attribute to ``True``.  This can have significant performance implications including some performance degradations in comparison to prior SQLAlchemy versions.  Dialect maintainers should seek to set this attribute to True after appropriate development and testing for SQLAlchemy 1.4 caching support.   Alternatively, this attribute may be set to False which will disable this warning.

All our engine dialects inherit from base classes that include supports_statement_cache = True, but as of v1.4.28 (not 1.4.5 as it says in the docs), sqlalchemy requires third party dialects to explicitly declare that too.

As I'm not entirely sure what we need to do to check and test that our subclassed dialects fully support caching, I've just pinned sqlalchemy for now, and I'll make another ticket for the caching support.

Copy link
Contributor

@evansd evansd left a comment

Choose a reason for hiding this comment

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

Great, thanks for getting to the bottom of this.

On the SQLAlchemy thing, I think the right thing to do here is to add

supports_statement_cache = True

to all our dialect subclasses. From looking at the the docs you linked to it seems to be a question of whether the dialect hardcodes literal parameters into SQL strings during compilation, and we're not doing anything like that. But in any case, you're right to address this in a separate ticket.

Another thing we should look at is testing our databricks compatibility. Unfortunately we can't do this in CI, but Simon has automated this as far as possible and written some detailed instructions.

I notice we're jumping from v0.9.1 to v2.0.2 of databricks-sql-connector so now would be a good time to do this. But I don't think testing should block this PR though. We may well have already broken compatibility with all the refactoring we've been doing, so we should move to the latest version and then figure out how to make that work — if it doesn't already.

@rebkwok
Copy link
Contributor Author

rebkwok commented Jul 5, 2022

I notice we're jumping from v0.9.1 to v2.0.2 of databricks-sql-connector so now would be a good time to do this

Ah - I did notice this, but I didn't realise that bit wasn't tested fully. I'll have a look at it separately to this PR. I think the docs are out of date too, the test referred to there doesn't exist

@rebkwok
Copy link
Contributor Author

rebkwok commented Jul 5, 2022

Sorry @evansd - I'd missed the requirements.dev.txt from the previous commit, and based on discussion in this thread I think the empty requirements.prod.in might be the thing stopping dependabot from working properly for prod requirements, so I've deleted it

@evansd
Copy link
Contributor

evansd commented Jul 5, 2022

I think the empty requirements.prod.in might be the thing stopping dependabot from working properly

Ah, interesting

@rebkwok
Copy link
Contributor Author

rebkwok commented Jul 5, 2022

But actually, maybe not. In any case, it probably makes sense to remove the empty requirements.prod.in since dependabot tries to be clever with .in files

@rebkwok rebkwok merged commit d3b5f03 into main Jul 5, 2022
@rebkwok rebkwok deleted the upgrade-requirements branch July 5, 2022 11:13
@rebkwok rebkwok mentioned this pull request Jul 5, 2022
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.

Review Dependabot security alerts
2 participants