Replace alchy with sqlservice and unfreeze SQLAlchemy #255

northwestwitch · 2024-05-17T07:45:34Z

This PR adds | fixes:

Fix Replace deprecated Alchy with SQLAlchemy #251

How to prepare for test:

Install in a local conda env -> pip install -e . (I've tested it with python=3.8)

How to test:

chanjo init --auto demodata
Make sure it has created a directory demodata containing these 3 files:

Link sample to stats -> chanjo --config demodata/chanjo.yaml link demodata/hgnc.grch37p13.exons.bed
Load config stats for sample1 ->chanjo --config demodata/chanjo.yaml load chanjo/init/demo-files/sample1.coverage.bed

Expected outcome:

Get samples mean coverage -> chanjo --config demodata/chanjo.yaml calculate mean -s sample1

Review:

Code approved by DN
Tests executed by CR, DN
"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

northwestwitch · 2024-05-17T08:33:21Z

chanjo/cli/db.py

-        context.abort()
-    LOG.info("delete sample (%s) from database", sample_id)
-    store.session.delete(sample_obj)
-    store.save()


store.save() is no longer required, read below

northwestwitch · 2024-05-17T08:41:13Z

chanjo/cli/load.py

    except IntegrityError:
-        LOG.exception('elements already linked?')
-        chanjo_db.session.rollback()


Rollback is handled automatically by the session, see here

northwestwitch · 2024-05-17T08:42:48Z

chanjo/store/api.py

@@ -105,21 +100,3 @@ def tear_down(self):
        # drop/delete the tables
        self.drop_all()
        return self
-
-    def save(self):


This is no longer required because sessions handle commits and rollbacks automatically

northwestwitch · 2024-05-17T08:53:40Z

tests/store/test_store_api.py

@@ -23,40 +22,14 @@ def test_no_dialect():
    assert chanjo_db.dialect == "sqlite"


-def test_save(chanjo_db):


This was testing a function that is now removed

northwestwitch · 2024-05-17T08:55:19Z

I've tested it locally and got the same result as with master brach. Also all automatic tests pass. Marking it ready for review!

dnil

Well done! Looks good to me, and tested as suggested locally without issue.

dnil · 2024-05-17T11:00:00Z

chanjo/store/models.py

@@ -2,14 +2,13 @@
 from collections import namedtuple
 from datetime import datetime

-from alchy import ModelBase, make_declarative_base
+from sqlservice import declarative_base


dnil · 2024-05-17T11:00:16Z

chanjo/store/models.py

 from sqlalchemy import Column, types, ForeignKey, UniqueConstraint, orm

 Exon = namedtuple('Exon', ['chrom', 'start', 'end', 'completeness'])

 # base for declaring a mapping
-BASE = make_declarative_base(Base=ModelBase)
-
+BASE = declarative_base()


chanjo/store/models.py

dnil · 2024-05-17T11:01:03Z

chanjo/store/models.py

@@ -89,11 +87,13 @@ class TranscriptStat(BASE):
    threshold = Column(types.Integer)
    _incomplete_exons = Column(types.Text)

-    sample_id = Column(types.String(32), ForeignKey('sample.id'),
+    sample_id = Column(types.String(32), ForeignKey('sample.id', ondelete='CASCADE'),


Same there, nice!

without that param if you removed a sample it didin't remove the linked records in TranscriptStat any more. I guess something has changed in SQLAlchemy when switching to version 2?

dnil · 2024-05-17T11:01:18Z

tests/cli/test_cli_calculate.py

-    data = json.loads(lines[0].strip())
-    assert data['sample_id'] == 'sample'
-    assert isinstance(data['mean_coverage'], float)
+    with popexist_db.begin() as session:


chanjo/store/models.py

northwestwitch · 2024-05-22T07:58:31Z

Testing on hasta stage:

First attempt failed, since SQLAlchemy 2.0 runs on python>=3.7 and the S_chanjo env had python 3.6.9
After updating the python version in S_chanjo to 3.11:

This branch now installs fine

$ bash /home/proj/production/servers/resources/hasta.scilifelab.se/update-tool-stage.sh -e S_chanjo -t chanjo -b deprecate_alchy

northwestwitch · 2024-05-22T08:24:26Z

Test loading coverage for a case:

One of the samples was already loaded but the other 2 got loaded.

The report looks like this:

Chiara Rasi added 24 commits May 16, 2024 09:51

Removed Alchy and fixed store.api to work with sqlservice

fc69035

Removed unused code

9e8cfac

Creates the db on file now

c7995ff

Loads transcripts without erroers

f2d7320

Fixed also loading sample

e1ba4d5

Fix code

e33c372

Fix connection

75669ed

Fixed mean and its test

2af3942

Fix calculate coverage and its test

8ae70e4

Fixed one more test

dab536e

Fixed another test

76ce1ed

Fixed test_link test

a5fa98c

Fixed test_setup_reset

ab23676

Fixed remove and test_remove

18839a3

Fix delete samples and test

ae47d24

Fix delete_group and its test

cdc10dd

Fixed test_dialect

13c1d51

Work in progress

4126ae8

Fixed test_add_many

e09a869

Fixed tests/test_calculate, test_mean

dc943ae

Fixed tests/test_calculate, test_mean_with_samples

1af7e9b

Fixed tests/test_calculate, test_gene

5f72e17

Fixed tests/test_calculate, test_sample_coverage

02aafcb

Fixed delete and test test_delete_sample

72bbe15

northwestwitch added enhancement needed wanted labels May 17, 2024

northwestwitch commented May 17, 2024

View reviewed changes

Small fix in model

5276700

northwestwitch commented May 17, 2024

View reviewed changes

northwestwitch marked this pull request as ready for review May 17, 2024 08:55

northwestwitch changed the title ~~Replace alchy with sqlservice~~ Replace alchy with sqlservice and unfreeze SQLAlchemy May 17, 2024

Updated changelog

f945a65

dnil approved these changes May 17, 2024

View reviewed changes

northwestwitch commented May 17, 2024

View reviewed changes

chanjo/store/models.py Outdated Show resolved Hide resolved

Clinical-Genomics deleted a comment from dnil May 17, 2024

Chiara Rasi added 2 commits May 20, 2024 13:18

Revert the naming of a key because then it's not found in chajo-report

3790ac7

Test something to fix chanjo-report

f8d3e60

northwestwitch mentioned this pull request May 20, 2024

Remove alchy and use SQLAlchemy instead Clinical-Genomics/chanjo-report#59

Merged

6 tasks

One last check to see if original model still works with chanjo-report

1d9f08f

northwestwitch mentioned this pull request May 21, 2024

Test any chanjo-report branch Clinical-Genomics/scout#4635

Closed

2 tasks

Force update the libs if they are old

81a5e56

northwestwitch mentioned this pull request May 22, 2024

Fix MT-report crash when one of the samples is not in chanjo db Clinical-Genomics/scout#4638

Merged

3 tasks

northwestwitch merged commit a2fb2d0 into master May 22, 2024
1 check passed

northwestwitch deleted the deprecate_alchy branch May 22, 2024 08:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace alchy with sqlservice and unfreeze SQLAlchemy #255

Replace alchy with sqlservice and unfreeze SQLAlchemy #255

northwestwitch commented May 17, 2024 •

edited

Loading

northwestwitch May 17, 2024 •

edited

Loading

northwestwitch May 17, 2024

northwestwitch May 17, 2024 •

edited

Loading

northwestwitch May 17, 2024

northwestwitch commented May 17, 2024

dnil left a comment

dnil May 17, 2024

dnil May 17, 2024

dnil May 17, 2024

northwestwitch May 17, 2024

dnil May 17, 2024

northwestwitch commented May 22, 2024

northwestwitch commented May 22, 2024 •

edited

Loading

		@@ -23,40 +22,14 @@ def test_no_dialect():
		assert chanjo_db.dialect == "sqlite"


		def test_save(chanjo_db):

Replace alchy with sqlservice and unfreeze SQLAlchemy #255

Replace alchy with sqlservice and unfreeze SQLAlchemy #255

Conversation

northwestwitch commented May 17, 2024 • edited Loading

This PR adds | fixes:

How to test:

Expected outcome:

Review:

northwestwitch May 17, 2024 • edited Loading

Choose a reason for hiding this comment

northwestwitch May 17, 2024

Choose a reason for hiding this comment

northwestwitch May 17, 2024 • edited Loading

Choose a reason for hiding this comment

northwestwitch May 17, 2024

Choose a reason for hiding this comment

northwestwitch commented May 17, 2024

dnil left a comment

Choose a reason for hiding this comment

dnil May 17, 2024

Choose a reason for hiding this comment

dnil May 17, 2024

Choose a reason for hiding this comment

dnil May 17, 2024

Choose a reason for hiding this comment

northwestwitch May 17, 2024

Choose a reason for hiding this comment

dnil May 17, 2024

Choose a reason for hiding this comment

northwestwitch commented May 22, 2024

northwestwitch commented May 22, 2024 • edited Loading

Test loading coverage for a case:

northwestwitch commented May 17, 2024 •

edited

Loading

northwestwitch May 17, 2024 •

edited

Loading

northwestwitch May 17, 2024 •

edited

Loading

northwestwitch commented May 22, 2024 •

edited

Loading