Better caching #421

tomaspavlin · 2021-02-03T10:51:15Z

Dřív jsme při jakémkoli editu pacientů mazali všechny konfigurace, teď zavádíme hash pacientů a ukládáme ji do db s konfigurací. Při fetchováním předpočítaných matchingů kontrolujeme tuto hash. Konfigurace nemažeme.

Testing

Několik unittestů a pak jsem to testoval v UI. Změnil jsem nastavení donora nebo recipienta -> vypočítaný matching se mi nezobrazil, změnil jsem nastavení zpět -> matching se zobrazil.

Closes #394

kubantjan

Vypada moc fajn, male komentare :)

kubantjan · 2021-02-03T14:00:19Z

txmatching/database/services/patient_service.py

    db.session.commit()
    return get_donor_from_donor_model(DonorModel.query.get(donor_update_dto.db_id))


+def get_patients_hash(txm_event: TxmEvent) -> int:
+    donors = tuple(txm_event.active_donors_dict.values()) \
+        if txm_event.active_donors_dict is not None else None


tohle se nikdy nemuze stat, viz ten post init v txm eventu. V tom txm eventu to vlastne jde prepsat tak jak jsi zminoval jinde, pres to init=false, tady to dava dobry smysl.

kubantjan · 2021-02-03T14:08:47Z

txmatching/database/services/patient_service.py

+
+# pylint: disable=too-many-branches, too-many-statements
+# Many branches and statements are still readable here
+def _update_hash(hash_, value):


Bylo by fajn tady dodat proc je to potreba delat takhle. Fakt nejde naimplementovat hashovani u tech objektu primo?

koukam jeste na https://stackoverflow.com/questions/52390576/how-can-i-make-a-python-dataclass-hashable

a uvedomuju si, ze nekde v kodu mame unsafe_hash = true, tam by asi bylo lepsi dat custom fci, at je to spravne safe a podobne by se to dalo dodelat i tady ne?

Proč to takhle dělám popisuji tady v Teamsech. Hashování s hash() funkcema by šlo docílit pomocí přenastavení PYTHONHASHSEED, ale nepřipadá mi to jako good practise. Takhle navíc můžeme hashovat jen to, na čem může být závislý výsledek algoritmu, tedy si myslím, že se může hodit to mít udělané takhle ručně a na jednom místě.

ok a jeste je tu teda nebezpeci, ze kdyz se zmeni libovolny objekt, ktery se tady resi, tak ze bude potreba zmenit kod tady, na coz hrozi, ze zapomeneme,

neslo by to tak, ze bys tu funkci persistent hash napsal u tech objektu primo? pak clovek kdyz meni ten objekt, rovnou tam uvidi, tu has fci a je to takovy, ze se na to spis nezapomene.

tomaspavlin · 2021-02-03T17:21:40Z

Zapracoval jsem připomínky @kubantjan

kubantjan · 2021-02-04T08:48:17Z

txmatching/database/services/patient_service.py

+
+# pylint: disable=too-many-branches, too-many-statements
+# Many branches and statements are still readable here
+def _update_hash(hash_, value):


ok a jeste je tu teda nebezpeci, ze kdyz se zmeni libovolny objekt, ktery se tady resi, tak ze bude potreba zmenit kod tady, na coz hrozi, ze zapomeneme,

neslo by to tak, ze bys tu funkci persistent hash napsal u tech objektu primo? pak clovek kdyz meni ten objekt, rovnou tam uvidi, tu has fci a je to takovy, ze se na to spis nezapomene.

kubantjan

Znova koukam na ten kod co se tyka hashovani a uvedomil jsem si zas jednu jinou vec: kdyz clovek zmeni hashovani, tak zmeni vlastne vsechny historicky pocitani pacientu, takze se potencialne to cely muze rozbit. Takze zmena hash by mela bejt promyslena a asi znamenat promazani historicky databaze, nebo zarizeni, ze ten vysledny hash bude furt stejny.

Ta verze, kterou jsi ted delal je dobra v tom smyslu, ze mozna budeme upravovat objekt pacienta, ale muzeme se mozna snazit ponechat ten hash bez zmeny co to jde. All in all, furt bych tu fci dal primo k tem objektum, nebo tam dal ten assert, ale asi bych nakonec tu hash neprepisoval, ptze ty pozadavky na tohle hashovani a to normalni se fakt nekdy muzou lisit a je nebezpecny, ze clovek bude predpokladat standartni chovani hash a to nenastane.

Jinak bylo by moc fajn tohle mergnout co nejdriv a nasadit v IKEM, ptze oni tam chteji mit dalsi fazi testovani.Jak se k tomu dostanes? Pokud by ses k tomu v nedeli/pondeli dopol nedostaval, tak bych skoro mergnul tu aktualni verzi a pak iteroval, at zrovna tohleto mame ready.

kubantjan · 2021-02-07T06:41:03Z

txmatching/database/services/patient_service.py

+            _update_hash(hash_, value.donor_type)
+            _update_hash(hash_, value.active)
+        if isinstance(value, Recipient):
+            _update_hash(hash_, value.related_donor_db_id)


kdyz jsi mluvil o ty performance, ze to potencialne muze bejt pomalejsi, testoval jsi, jak dlouho tohle zabere pro 100 pacientu treba?

asi by bylo fajn to vyzkouset,

pokud byc to byl netrivialni cas, asi by pomohlo dat nejdriv vsechny ty hodnoty z treba patient parameters do kupy a volat na to jeden _update_has (ale pokud je to rychly, tak to resit neni treba..)

vyzkouším

Creating persistent hash of 72 patients took about 0.014 seconds

tomaskourim

Par otazek a par drobnosti.

tomaskourim · 2021-02-07T17:22:44Z

txmatching/database/services/patient_upload_service.py

@@ -35,7 +33,6 @@


 def add_donor_recipient_pair(donor_recipient_pair_dto: DonorRecipientPairDTO, txm_event_db_id: int):
-    remove_configs_from_txm_event(txm_event_db_id)


muze se rovnou odstranit i ta funkce, uz se nepouzije

tomaskourim · 2021-02-07T17:23:58Z

txmatching/database/sql_alchemy_schema.py

@@ -22,6 +22,7 @@ class ConfigModel(db.Model):
    id = db.Column(Integer, primary_key=True, autoincrement=True, nullable=False)
    txm_event_id = db.Column(db.Integer, ForeignKey('txm_event.id', ondelete='CASCADE'), unique=False, nullable=False)
    parameters = db.Column(db.JSON, unique=False, nullable=False)
+    patients_hash = db.Column(db.BigInteger, unique=False, nullable=False)


fakticky se tohle prida jako posledni sloupecek. Nevim jestli neni lepsi ho mit i zde jako posledni

tomaskourim · 2021-02-07T17:27:12Z

txmatching/web/api/matching_api.py

@@ -64,4 +64,5 @@ def post(self, txm_event_id: int) -> str:
                                                            :configuration.max_matchings_to_show_to_viewer]
            calculated_matchings_dto.show_not_all_matchings_found = False

+        calculated_matchings_dto.calculated_matchings = calculated_matchings_dto.calculated_matchings


to je identita, ne?

tomaskourim · 2021-02-07T17:38:34Z

txmatching/database/services/patient_service.py

+    recipients = tuple(txm_event.active_recipients_dict.values())
+
+    start = time.time()
+    hash_ = initialize_persistent_hash()


ma to oznaceni _ zde nejaky vyznam? Jmeno treba persistent_hash by mi pripadalo lespi mozna. Pripadne bych upravil vsude.

hash je build-in name. persistent_hash mi připadá možná trochu dlouhé. To třeba při použití v update_persistant_hash(), kde je označení hash_ zcela jasné. Co myslíš?

tomaskourim · 2021-02-07T17:39:01Z

txmatching/database/services/patient_service.py

+    hash_digest = get_hash_digest(hash_)
+    end = time.time()
+
+    logger.debug(f'Creating persistent hash of {len(donors) + len(recipients)} patients took {end - start} seconds')


trva to dlouho?

Netrvá, dám pryč

tomaskourim · 2021-02-07T17:49:00Z

txmatching/patients/patient.py

+        update_persistent_hash(hash_, Donor)
+        update_persistent_hash(hash_, self.related_recipient_db_id)
+        update_persistent_hash(hash_, self.donor_type)
+        update_persistent_hash(hash_, self.active)


kdyz delat hash pro config a txm event, tak beres jenom active donory. Ale myslim, ze by to nemelo hrat roli. Potencialne duplicita, ale podle me nevadi. (Jenom ze jsem si vsimnul)

ano, je to duplicita, ale připadá mi to takhle jistější a blbuvzdornější

tomaskourim · 2021-02-07T17:53:56Z

txmatching/patients/patient.py


-@dataclass
+
+@dataclass(init=False)


tenhle trik bych chtel vysvetlit :)

protože definuji __init__ funkci ručně, nepotřebujeme ji nechat vytvářet automaticky. Použití init místo post_init je lepší v tom, že ty dva fieldy nemusí být optional

tomaskourim · 2021-02-07T17:56:25Z

txmatching/patients/hla_model.py

@@ -46,9 +57,13 @@ def __post_init__(self):
            hla_types = [hla_type for hla_type in self.hla_types_list if hla_type.code]
            self.hla_per_groups = _split_hla_types_to_groups(hla_types)

+    def update_persistent_hash(self, hash_):
+        update_persistent_hash(hash_, HLATyping)
+        update_persistent_hash(hash_, self.hla_per_groups)


a hla_types_list nechceme, protoze vlastne obsahuje stejne udaje, jenom neni roztrideny?

hlavně jestli se nepletu tak vstup do solveru je hla_per_groups a ne hla_types_list

jop tak tak

tomaskourim · 2021-02-07T18:03:04Z

tests/utils/test_matching.py

@@ -24,107 +21,9 @@
    'DR15'
 ]

-DONORS = [


jako proc ne, ale presouval bych tedy vsechno nebo nic. Takhle tu par konstant zustalo, par ne. Idealne aby tahle data byla na jednom miste, podle me.

tomaskourim · 2021-02-07T18:12:08Z

tests/database/services/test_patient_service.py

+        hash_1 = get_patients_persistent_hash(txm_event_1)
+        self.assertEqual(hash_1, 1602329590066289936)
+
+        # changing event db id, event name does not change the hash


changing event db id OR event name does not change the hash

Aby to bylo patrnejsi, chvili jsem dumal

kubantjan · 2021-02-08T12:35:43Z

txmatching/patients/hla_model.py

@@ -46,9 +57,13 @@ def __post_init__(self):
            hla_types = [hla_type for hla_type in self.hla_types_list if hla_type.code]
            self.hla_per_groups = _split_hla_types_to_groups(hla_types)

+    def update_persistent_hash(self, hash_):


jeste prosim jen typ hash_ napsat

to moc dobře nejde. Typ je hashlib._Hash a jednoduše s ním anotovat nejde. Připadá mi, že nestojí za to se to snažit rozchodit python/typeshed#2928

kubantjan

Jen jeste chybejici typ u has_ prosim doplnit typovani.

u value asi patri proste any.

# Conflicts: # txmatching/web/api/matching_api.py

I checked and everything is resolved

tomaspavlin added 9 commits February 2, 2021 11:19

Introduce patient hash (1)

86d5a49

Introduce patient hash (2)

ea624fb

Add hash to db and use it

65e01cf

Create patient hash using hashlib

7a52e14

Solve some todos

e67a09f

Fix tests

73b84e5

Fix lint

4e09b23

Last changes

c690f6c

Merge remote-tracking branch 'origin/master' into 394_better_caching

9210d79

tomaspavlin requested review from kubantjan and tomaskourim February 3, 2021 10:51

tomaspavlin self-assigned this Feb 3, 2021

tomaspavlin mentioned this pull request Feb 3, 2021

Better caching #420

Closed

kubantjan requested changes Feb 3, 2021

View reviewed changes

Apply CR comments

36e0471

tomaspavlin requested a review from kubantjan February 3, 2021 17:21

Fix test

d0441f3

kubantjan requested changes Feb 4, 2021

View reviewed changes

kubantjan requested changes Feb 7, 2021

View reviewed changes

tomaspavlin added 4 commits February 7, 2021 13:09

Move persistent hashing functionality to classes

4fb845e

Measure hashing time

bfa17d4

Merge remote-tracking branch 'origin/master' into 394_better_caching

3389e8a

Fix merge

4b00d31

tomaspavlin requested a review from kubantjan February 7, 2021 13:30

tomaskourim previously requested changes Feb 7, 2021

View reviewed changes

tomaskourim added the waiting for the author Waiting for author to respond label Feb 7, 2021

tomaspavlin added 2 commits February 7, 2021 23:20

Apply TK comments

dc392ef

Move testing objects

9a27634

kubantjan reviewed Feb 8, 2021

View reviewed changes

kubantjan requested changes Feb 8, 2021

View reviewed changes

tomaspavlin added 2 commits February 8, 2021 14:37

Merge remote-tracking branch 'origin/master' into 394_better_caching

70e60ef

# Conflicts: # txmatching/web/api/matching_api.py

Annote hash type

8406f36

tomaspavlin requested a review from kubantjan February 8, 2021 14:46

kubantjan approved these changes Feb 8, 2021

View reviewed changes

kubantjan merged commit f511b36 into master Feb 8, 2021

kubantjan deleted the 394_better_caching branch February 8, 2021 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better caching #421

Better caching #421

tomaspavlin commented Feb 3, 2021 •

edited

Loading

kubantjan left a comment

kubantjan Feb 3, 2021

kubantjan Feb 3, 2021

tomaspavlin Feb 3, 2021 •

edited

Loading

kubantjan Feb 4, 2021

tomaspavlin commented Feb 3, 2021

kubantjan Feb 4, 2021

kubantjan left a comment

kubantjan Feb 7, 2021

tomaspavlin Feb 7, 2021

tomaspavlin Feb 7, 2021

tomaskourim left a comment

tomaskourim Feb 7, 2021

tomaskourim Feb 7, 2021

tomaskourim Feb 7, 2021

tomaspavlin Feb 7, 2021

tomaskourim Feb 7, 2021

tomaspavlin Feb 7, 2021

tomaskourim Feb 7, 2021

tomaspavlin Feb 7, 2021

tomaskourim Feb 7, 2021

tomaspavlin Feb 7, 2021

tomaskourim Feb 7, 2021

tomaspavlin Feb 7, 2021

tomaskourim Feb 7, 2021

tomaspavlin Feb 7, 2021

kubantjan Feb 8, 2021

tomaskourim Feb 7, 2021

tomaskourim Feb 7, 2021

kubantjan Feb 8, 2021

tomaspavlin Feb 8, 2021

kubantjan left a comment

		@@ -35,7 +33,6 @@


		def add_donor_recipient_pair(donor_recipient_pair_dto: DonorRecipientPairDTO, txm_event_db_id: int):
		remove_configs_from_txm_event(txm_event_db_id)


		@dataclass

		@dataclass(init=False)

Better caching #421

Better caching #421

Conversation

tomaspavlin commented Feb 3, 2021 • edited Loading

Testing

kubantjan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaspavlin Feb 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaspavlin commented Feb 3, 2021

Choose a reason for hiding this comment

kubantjan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaskourim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubantjan left a comment

Choose a reason for hiding this comment

tomaspavlin commented Feb 3, 2021 •

edited

Loading

tomaspavlin Feb 3, 2021 •

edited

Loading