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

Unfreeze SQLAlchemy #252

Closed
wants to merge 13 commits into from
Closed

Unfreeze SQLAlchemy #252

wants to merge 13 commits into from

Conversation

dnil
Copy link
Collaborator

@dnil dnil commented May 13, 2024

This PR adds | fixes:

How to prepare for test:

  • ssh to ...
  • Install on stage:
    bash servers/resources/SERVER.scilifelab.se/update-[THIS_TOOL]-stage.sh [THIS-BRANCH-NAME]

How to test:

Expected outcome:

  • [ ]

Review:

  • Code approved by
  • Tests executed by
  • "Merge and deploy" approved by

This version is a:

  • MAJOR - when you make incompatible API changes
  • MINOR - when you add functionality in a backwards compatible manner
  • PATCH - when you make backwards compatible bug fixes or documentation/instructions

@dnil dnil requested a review from northwestwitch May 13, 2024 07:58
Copy link
Member

@northwestwitch northwestwitch left a comment

Choose a reason for hiding this comment

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

Seems a risky change to me

@@ -6,4 +6,4 @@ toolz
ruamel.yaml
importlib_metadata
pymysql
sqlalchemy==1.3.*
Copy link
Member

Choose a reason for hiding this comment

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

sqlalchemy 2 can be quite different from v.1, there is even a migration guide: https://docs.sqlalchemy.org/en/20/changelog/migration_20.html

Just unfreezing like this seems risky..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I realised a little while ago. 😊 It was useful to test the dependencies at least: nice to see that chanjo-report / scout did build and run ok with this configuration. But chanjo would likely bug out without changes..

@@ -4,7 +4,7 @@
from datetime import datetime

import pytest
from sqlite3 import IntegrityError
from sqlalchemy.exc import IntegrityError
Copy link
Member

Choose a reason for hiding this comment

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

💯

Copy link
Member

@northwestwitch northwestwitch left a comment

Choose a reason for hiding this comment

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

Don't forget to update the CHANGELOG. Then we could release a new version of chanjo perhaps?

@dnil
Copy link
Collaborator Author

dnil commented May 13, 2024

Mm, yes, but this is only to 1.4 so far. If you dont mind I would like to enable move-to-2.0-warnings and see what gives.
From the changelog its also evident we have actually already been higher than 1.3, although its not super clear what version we were on..

@northwestwitch
Copy link
Member

Mm, yes, but this is only to 1.4 so far. If you dont mind I would like to enable move-to-2.0-warnings and see what gives. From the changelog its also evident we have actually already been higher than 1.3, although its not super clear what version we were on..

With SQLAlchemy 1.4 you can try creating a "future" engine, something that resembles SQLAlchemy 2. Perhaps this would help? --> https://docs.sqlalchemy.org/en/14/core/future.html

@dnil
Copy link
Collaborator Author

dnil commented May 13, 2024

Well, yes, I tested with

SQLALCHEMY_WARN_20=1 pytest tests/

It gives a few, but it looks like only a few functions. I suspect the calculate -> add_columns will be the most annoying..

../../../../usr/local/miniconda3/envs/scout38again/lib/python3.8/site-packages/alchy/model.py:421
  /usr/local/miniconda3/envs/scout38again/lib/python3.8/site-packages/alchy/model.py:421: MovedIn20Warning: The ``declarative_base()`` function is now available as sqlalchemy.orm.declarative_base(). (deprecated since: 1.4) (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
    Model = declarative_base(cls=Base,

../../../../usr/local/miniconda3/envs/scout38again/lib/python3.8/site-packages/_pytest/config/__init__.py:1448
  /usr/local/miniconda3/envs/scout38again/lib/python3.8/site-packages/_pytest/config/__init__.py:1448: PytestConfigWarning: Unknown config option: looponfailroots

    self._warn_or_fail_if_strict(f"Unknown config option: {key}\n")

tests/cli/test_cli_calculate.py: 16 warnings
tests/cli/test_cli_load.py: 20 warnings
tests/cli/test_cli_store.py: 69 warnings
tests/store/test_store_api.py: 81 warnings
tests/test_calculate.py: 32 warnings
  /usr/local/miniconda3/envs/scout38again/lib/python3.8/site-packages/alchy/session.py:31: SADeprecationWarning: Use .persist_selectable (deprecated since: 1.3)
    info = getattr(mapper.mapped_table, 'info', {})

tests/cli/test_cli_store.py::test_remove
  /Users/dannil/sandbox/chanjo/tests/cli/test_cli_store.py:32: LegacyAPIWarning: The Query.get() method is considered legacy as of the 1.x series of SQLAlchemy and becomes a legacy construct in 2.0. The method is now available as Session.get() (deprecated since: 1.4) (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
    assert Sample.query.get(sample_id)

tests/cli/test_cli_store.py::test_remove
tests/cli/test_cli_store.py::test_remove
  /Users/dannil/sandbox/chanjo/chanjo/cli/db.py:40: LegacyAPIWarning: The Query.get() method is considered legacy as of the 1.x series of SQLAlchemy and becomes a legacy construct in 2.0. The method is now available as Session.get() (deprecated since: 1.4) (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
    sample_obj = Sample.query.get(sample_id)

tests/cli/test_cli_store.py::test_remove
  /Users/dannil/sandbox/chanjo/tests/cli/test_cli_store.py:37: LegacyAPIWarning: The Query.get() method is considered legacy as of the 1.x series of SQLAlchemy and becomes a legacy construct in 2.0. The method is now available as Session.get() (deprecated since: 1.4) (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
    assert Sample.query.get(sample_id) is None

tests/cli/test_cli_store.py::test_samples
  /Users/dannil/sandbox/chanjo/tests/cli/test_cli_store.py:52: LegacyAPIWarning: The Query.get() method is considered legacy as of the 1.x series of SQLAlchemy and becomes a legacy construct in 2.0. The method is now available as Session.get() (deprecated since: 1.4) (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
    assert Sample.query.get(sample_id)

tests/cli/test_cli_store.py::test_transcripts
  /Users/dannil/sandbox/chanjo/tests/cli/test_cli_store.py:78: LegacyAPIWarning: The Query.get() method is considered legacy as of the 1.x series of SQLAlchemy and becomes a legacy construct in 2.0. The method is now available as Session.get() (deprecated since: 1.4) (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
    assert Sample.query.get(sample_id)

tests/cli/test_cli_store.py::test_delete
  /Users/dannil/sandbox/chanjo/tests/cli/test_cli_store.py:105: LegacyAPIWarning: The Query.get() method is considered legacy as of the 1.x series of SQLAlchemy and becomes a legacy construct in 2.0. The method is now available as Session.get() (deprecated since: 1.4) (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
    assert Sample.query.get(sample_id)

tests/cli/test_cli_store.py::test_delete
  /Users/dannil/sandbox/chanjo/tests/cli/test_cli_store.py:113: LegacyAPIWarning: The Query.get() method is considered legacy as of the 1.x series of SQLAlchemy and becomes a legacy construct in 2.0. The method is now available as Session.get() (deprecated since: 1.4) (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
    assert Sample.query.get(sample_id) is None

tests/cli/test_cli_store.py::test_delete_group
  /Users/dannil/sandbox/chanjo/tests/cli/test_cli_store.py:121: LegacyAPIWarning: The Query.get() method is considered legacy as of the 1.x series of SQLAlchemy and becomes a legacy construct in 2.0. The method is now available as Session.get() (deprecated since: 1.4) (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
    assert Sample.query.get(sample_id)

tests/cli/test_cli_store.py::test_delete_group
  /Users/dannil/sandbox/chanjo/tests/cli/test_cli_store.py:129: LegacyAPIWarning: The Query.get() method is considered legacy as of the 1.x series of SQLAlchemy and becomes a legacy construct in 2.0. The method is now available as Session.get() (deprecated since: 1.4) (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
    assert Sample.query.get(sample_id) is None

tests/store/test_store_api.py::test_save
  /Users/dannil/sandbox/chanjo/tests/store/test_store_api.py:36: LegacyAPIWarning: The Query.get() method is considered legacy as of the 1.x series of SQLAlchemy and becomes a legacy construct in 2.0. The method is now available as Session.get() (deprecated since: 1.4) (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
    assert Sample.query.get(sample_id) == new_sample

tests/store/test_store_api.py::test_save
  /Users/dannil/sandbox/chanjo/chanjo/store/api.py:120: SAWarning: New instance <Sample at 0x7fcb29a117f0> with identity key (<class 'chanjo.store.models.Sample'>, ('ADM12',), None) conflicts with persistent instance <Sample at 0x7fcb29a11730>
    self.commit()

tests/store/test_store_api.py::test_save
  /Users/dannil/sandbox/chanjo/tests/store/test_store_api.py:49: LegacyAPIWarning: The Query.get() method is considered legacy as of the 1.x series of SQLAlchemy and becomes a legacy construct in 2.0. The method is now available as Session.get() (deprecated since: 1.4) (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
    assert Sample.query.get(new_sampleid)

tests/test_calculate.py::test_gene
  /Users/dannil/sandbox/chanjo/chanjo/calculate.py:30: SADeprecationWarning: Query.add_column() is deprecated and will be removed in a future release.  Please use Query.add_columns() (deprecated since: 1.4)
    self.mean()

@dnil
Copy link
Collaborator Author

dnil commented May 13, 2024

Ah, no, this will be the most annoying, since its actually not in our codebase.. 😞

../../../../usr/local/miniconda3/envs/scout38again/lib/python3.8/site-packages/alchy/model.py:421
  /usr/local/miniconda3/envs/scout38again/lib/python3.8/site-packages/alchy/model.py:421: MovedIn20Warning: The ``declarative_base()`` function is now available as sqlalchemy.orm.declarative_base(). (deprecated since: 1.4) (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
    Model = declarative_base(cls=Base

@dnil
Copy link
Collaborator Author

dnil commented May 13, 2024

Ok, that was useful on some level I suppose. I'm now fairly convinced we will not be able to use Alchy (final version 2.2.2) with SQLAlchemy>=2. 1.4 seems ok if we wish to go through the hassle; it might help a little somewhere, but it is not going to take us to higher versions of e.g. Flask-SQLAlchemy that we would like.

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

2 participants