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

Update SQLAlchemy import path #1251

Merged
merged 6 commits into from Feb 15, 2023
Merged

Update SQLAlchemy import path #1251

merged 6 commits into from Feb 15, 2023

Conversation

antonymilne
Copy link
Contributor

Closes #1244. As @astrojuanlu correctly observed, we haven't actually gotten rid of the deprecation warning, we just pinned the version. This alters the import path to actually fix it.

In fact, the version was already pinned before: #1222 didn't really do anything because the version specifier ~=1.4 is essentially the same as >=1.4, <2.0. So I'm just reverting that to the ~= we had before, because it's more common in our codebase. Looking at the SQLAlchemy migration guide we could unpin this completely and just do >=1.4 but I've left the pin there for now just to be on the safe side (in general, I'd like to move towards more open-ended upper bounds, but that's for another time).

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Thanks @AntonyMilneQB! Changes LGTM

Copy link
Contributor

@jmholzer jmholzer left a comment

Choose a reason for hiding this comment

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

Looks good, did you do a manual check that the deprecation warning is gone after these changes?

@antonymilne
Copy link
Contributor Author

@jmholzer yep, I did and it's gone.

@astrojuanlu
Copy link
Member

so, as @AntonyMilneQB spotted, there seems to be a problem with the types:

package/kedro_viz/models/experiment_tracking.py:11: error: Module "sqlalchemy.orm" has no attribute "declarative_base"  [attr-defined]

(and then a bunch of errors that cascade from there). This is essentially an instance of dropbox/sqlalchemy-stubs#250.

On the other hand, sqlalchemy 2.0 started shipping its own types sqlalchemy/sqlalchemy#6810 and I think sqlalchemy.orm is already covered, but we can't switch to it just yet.

The options are:

  • don't do anything until we upgrade to sqlalchemy 2.0. users will keep seeing the deprecation warning. the other problem is that this would require "jumping" version without a smoother transition path.
  • # type: ignore this error until we upgrade to sqlalchemy >= 2.0. introduces a small amount of risk because we would be "blind" to type errors around this area for a while. hopefully there are unit tests for it though?
  • not sure there's a third option.

@antonymilne
Copy link
Contributor Author

I find the warning sufficiently annoying that I think we should prioritise getting rid of it, since it appears not just when you run kedro viz but also when you run kedro jupyter commands if you have kedro-viz installed (due to the run_viz line magic being registered). Hence I'd be in favour of option 2. Risk of introducing bugs as a result of this seems very low, but we should keep an eye on sqlalchemy-stubs so that if there's a fix we can remove the ignore.

@tynandebold tynandebold closed this Feb 6, 2023
@tynandebold tynandebold reopened this Feb 6, 2023
@antonymilne antonymilne added this pull request to the merge queue Feb 15, 2023
Merged via the queue into main with commit cf91c95 Feb 15, 2023
@antonymilne antonymilne deleted the chore/sqlalchemy branch February 15, 2023 16:29
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.

Address deprecation warnings coming from SQLAlchemy 1.4
5 participants