DIG-1582: Implement overview thresholds & DIG-1533: Change cancer_type to primary_site #205

SonQBChau · 2024-05-10T05:46:43Z

What's new

Introduce a threshold mechanism to display masked values for small counts, e.g., "<5".
Rewrite queries for masking and improved performance.
Update return types from int to str to handle masked values.
Update schema return structure to accommodate new queries.
Rename discover_cancer_type_count to discover_primary_site_count
Use lazyFunction for enhanced random seeds in testing
Add a test suite to ensure proper functionality of overview masking for small counts.

Types of Change(s)

✨ New feature
💥 Breaking change

Which other services does it affect?

data-portal
query

Has it been tested for:

Locally tested
Docker tested

How to test

For local, run tox
For docker, pull this branch, add to docker-compose AGGREGATE_COUNT_THRESHOLD=${AGGREGATE_COUNT_THRESHOLD} and run test-integration
Optional: Add some data and call overview endpoints to see if value <5 gets properly masked.

- Introduced threshold for small counts to display masked values, e.g., <5. - Rewrite queries for masking and performance. - Updated return types from int to str to cope with "<5" - Update schema return structure for new queries

…G/katsu into sonchau/overview_threshold

codecov · 2024-05-10T05:49:37Z

Codecov Report

Attention: Patch coverage is 97.93103% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 96.69%. Comparing base (c05e921) to head (b72daf2).

Files	Patch %	Lines
...hord_metadata_service/mohpackets/apis/discovery.py	91.42%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #205      +/-   ##
===========================================
+ Coverage    95.11%   96.69%   +1.57%     
===========================================
  Files           34       35       +1     
  Lines         3503     3570      +67     
===========================================
+ Hits          3332     3452     +120     
+ Misses         171      118      -53

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

CourtneyGosselin

Approved! I was able to spin up the environment and test some of the calls. The results return as expected, showing the new AGGREGATE_COUNT_THRESHOLD <5

OrdiNeu · 2024-05-21T16:07:43Z

Getting an error on the frontpage with this PR, from Katsu's logs:

Cannot use None as a query value                                                                                                                                                                   [22/1852]
Traceback (most recent call last):                                                                                                                                                                          
  File "/usr/local/lib/python3.12/site-packages/ninja/operation.py", line 107, in run
    result = self.view_func(request, **values)   
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^                                                                                                                                                              
  File "/app/chord_metadata_service/chord_metadata_service/mohpackets/apis/discovery.py", line 302, in discover_diagnosis_age_count
    .annotate(                   
     ^^^^^^^^^                                                                                        
  File "/usr/local/lib/python3.12/site-packages/django/db/models/query.py", line 1630, in annotate
    return self._annotate(args, kwargs, select=True)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^                                                                                                                                                        
  File "/usr/local/lib/python3.12/site-packages/django/db/models/query.py", line 1680, in _annotate
    clone.query.add_annotation(                                                                       
  File "/usr/local/lib/python3.12/site-packages/django/db/models/sql/query.py", line 1198, in add_annotation
    annotation = annotation.resolve_expression(self, allow_joins=True, reuse=None)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/django/db/models/expressions.py", line 1513, in resolve_expression
    c.cases[pos] = case.resolve_expression(                                                                                                                                                                 
                   ^^^^^^^^^^^^^^^^^^^^^^^^                                                                                                                                                                   File "/usr/local/lib/python3.12/site-packages/django/db/models/expressions.py", line 1432, in resolve_expression
    c.condition = c.condition.resolve_expression(                                                                                                                                                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/django/db/models/query_utils.py", line 90, in resolve_expression                                                                               
    clause, joins = query._add_q(                                                                     
                    ^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/django/db/models/sql/query.py", line 1645, in _add_q
    child_clause, needed_inner = self.build_filter( 
                                 ^^^^^^^^^^^^^^^^^^ 
  File "/usr/local/lib/python3.12/site-packages/django/db/models/sql/query.py", line 1513, in build_filter
    condition = self.build_lookup(lookups, reffed_expression, value)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/django/db/models/sql/query.py", line 1394, in build_lookup
    raise ValueError("Cannot use None as a query value")
ValueError: Cannot use None as a query value
[21/May/2024 16:06:46] [django] ERROR: Cannot use None as a query value
Traceback (most recent call last):
  File "/usr/local/lib/python3.12/site-packages/ninja/operation.py", line 107, in run
    result = self.view_func(request, **values)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/chord_metadata_service/chord_metadata_service/mohpackets/apis/discovery.py", line 302, in discover_diagnosis_age_count
    .annotate(
     ^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/django/db/models/query.py", line 1630, in annotate
    return self._annotate(args, kwargs, select=True)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/django/db/models/query.py", line 1680, in _annotate
    clone.query.add_annotation(
  File "/usr/local/lib/python3.12/site-packages/django/db/models/sql/query.py", line 1198, in add_annotation
    annotation = annotation.resolve_expression(self, allow_joins=True, reuse=None)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/django/db/models/expressions.py", line 1513, in resolve_expression
    c.cases[pos] = case.resolve_expression(
                   ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/django/db/models/expressions.py", line 1432, in resolve_expression
    c.condition = c.condition.resolve_expression(
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/django/db/models/query_utils.py", line 90, in resolve_expression
    clause, joins = query._add_q(
                    ^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/django/db/models/sql/query.py", line 1645, in _add_q
    child_clause, needed_inner = self.build_filter(
                                 ^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/django/db/models/sql/query.py", line 1513, in build_filter
    condition = self.build_lookup(lookups, reffed_expression, value)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/django/db/models/sql/query.py", line 1394, in build_lookup
    raise ValueError("Cannot use None as a query value")
ValueError: Cannot use None as a query value
Internal Server Error: /v2/discovery/overview/diagnosis_age_count/
[21/May/2024 16:06:46] [django.request] ERROR: Internal Server Error: /v2/discovery/overview/diagnosis_age_count/
[pid: 16|app: 0|req: 43/43] 172.20.0.1 () {32 vars in 2040 bytes} [Tue May 21 16:06:46 2024] GET /v2/discovery/overview/diagnosis_age_count/ => generated 2247 bytes in 10 msecs (HTTP/1.1 500) 9 headers in 664 bytes (1 switches on core 0)

SonQBChau · 2024-05-21T16:43:08Z

I can confirm this bug, looking into it

SonQBChau · 2024-05-21T17:07:51Z

fixed. Can you check it again @OrdiNeu ?

OrdiNeu · 2024-05-21T17:41:39Z

I've rebuilt a few times with the latest changes but still get the same error listed above. For reference, I'm just visiting the frontpage and checking the logs -- it's a request straight to Katsu's /v2/discovery/overview/diagnosis_age_count/ endpoint.

SonQBChau · 2024-05-22T00:16:22Z

@OrdiNeu Have you added AGGREGATE_COUNT_THRESHOLD=${AGGREGATE_COUNT_THRESHOLD} to docker compose? (I put a note in the description, but even I forgot it myself).

To avoid this in the future, I have added the code to check if any .env is missing.

Let me know if it works for you.

OrdiNeu · 2024-05-22T14:58:02Z

Oh, that sort of works now. Getting a new error now though, on individual_count only:

'>=' not supported between instances of 'int' and 'str'
Traceback (most recent call last):
  File "/usr/local/lib/python3.12/site-packages/ninja/operation.py", line 107, in run
    result = self.view_func(request, **values)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/chord_metadata_service/chord_metadata_service/mohpackets/apis/discovery.py", line 188, in discover_individual_count
    if donor_count >= SMALL_NUMBER_THRESHOLD
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: '>=' not supported between instances of 'int' and 'str'
[22/May/2024 14:56:51] [django] ERROR: '>=' not supported between instances of 'int' and 'str'
Traceback (most recent call last):
  File "/usr/local/lib/python3.12/site-packages/ninja/operation.py", line 107, in run
    result = self.view_func(request, **values)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/chord_metadata_service/chord_metadata_service/mohpackets/apis/discovery.py", line 188, in discover_individual_count
    if donor_count >= SMALL_NUMBER_THRESHOLD
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: '>=' not supported between instances of 'int' and 'str'
Internal Server Error: /v2/discovery/overview/individual_count/
[22/May/2024 14:56:51] [django.request] ERROR: Internal Server Error: /v2/discovery/overview/individual_count/

- fix the bug where AGGREGATE_COUNT_THRESHOLD read as a str in docker env - should show 0 instead of <5 for value 0

SonQBChau · 2024-05-22T22:36:15Z

I fixed the bug where AGGREGATE_COUNT_THRESHOLD is not an int. Accidentally, I found a bug with cache but I will address it in another ticket

OrdiNeu

Looks good, well-written ✔️

The latest fix made it work for me

OrdiNeu · 2024-05-23T14:29:30Z

config/settings/prod.py

+    "REDIS_PASSWORD_FILE",
+]
+
+missing_vars = [var for var in required_env_vars if not os.getenv(var)]


This file switches between using os.getenv() and os.environ[] in several places -- I'd recommend sticking to one or the other (as far as I understand it, one's a wrapper of the other)

There is a slight difference, and it's on purpose. os.getenv returns None if the variable is not found, allowing me to catch it and throw an exception.

Why not use os.getenv below? If it returns None for a missing variable, it can cause the bug we encountered before. If I (or someone else) tries to get a new variable but forgets to add it to required_vars, it will crash instead of causing a subtle bug, making it easier to catch.

I see -- perhaps it would be best if lines 34-36 used os.getenv then, rather than os.environ.get()? For consistency

oh thank you for the eagle eye! There was a merge conflict and I used the upstream code without realized it

OrdiNeu · 2024-05-23T14:30:15Z

chord_metadata_service/mohpackets/tests/endpoints/test_overview.py

+"""
+
+
+class OverviewTestCase(BaseTestCase):


These tests are well-written 👍

OrdiNeu · 2024-05-23T14:31:32Z

chord_metadata_service/mohpackets/tests/endpoints/factories.py

-    }
+    date_of_birth = factory.LazyFunction(
+        lambda: {
+            "month_interval": random.randint(0, 1000),


Wouldn't this make the month_interval and day_interval not line up? E.g. you could have a day_interval of 0 and a month_interval of 1000.

You are correct, but I'm not testing the date logic here. I agree that a more realistic generator would be better, but these mock data serve my purpose for now.

Sounds good to me -- maybe document this discrepancy in the Note: at the top of this file. Otherwise it looks good ✔️

OrdiNeu · 2024-05-23T14:36:14Z

NB: we should probably hold on updating the submodule version on CanDIGv2's develop until we have both Query PR and the Data Portal PR merged at the same time. In addition, we need a second Data Portal PR to handle censoring.

- don't use get

SonQBChau and others added 9 commits May 9, 2024 22:39

Update discovery.py

ac05aef

- Introduced threshold for small counts to display masked values, e.g., <5. - Rewrite queries for masking and performance. - Updated return types from int to str to cope with "<5" - Update schema return structure for new queries

add new schema for overview masking count

6ee58bc

use lazyFunction for better random seeds

a0062ee

add test suite for overview masking small count

ceaba56

add AGGREGATE_COUNT_THRESHOLD var

0cfe1bf

update schemas

5a733de

Update schemas with SHA

deedea1

bump katsu version

ac47523

Merge branch 'sonchau/overview_threshold' of https://github.com/CanDI…

ddb30b0

…G/katsu into sonchau/overview_threshold

clean up

367246c

SonQBChau marked this pull request as ready for review May 10, 2024 18:11

SonQBChau requested review from yavyx, CourtneyGosselin and OrdiNeu May 10, 2024 18:11

CourtneyGosselin approved these changes May 16, 2024

View reviewed changes

mshadbolt changed the title ~~Sonchau/overview threshold~~ DIG-1582: overview threshold & DIG-1533: change cancer_type to primary_site May 21, 2024

mshadbolt changed the title ~~DIG-1582: overview threshold & DIG-1533: change cancer_type to primary_site~~ DIG-1582: Implement overview thresholds & DIG-1533: Change cancer_type to primary_site May 21, 2024

fix None value not a string

835452e

make required env check

2aca72e

SonQBChau added 2 commits May 21, 2024 18:23

Merge branch 'candig_develop' into sonchau/overview_threshold

9131331

clean up

adf85ca

fix discover_individual_count

aa07595

- fix the bug where AGGREGATE_COUNT_THRESHOLD read as a str in docker env - should show 0 instead of <5 for value 0

OrdiNeu approved these changes May 23, 2024

View reviewed changes

SonQBChau added 2 commits May 23, 2024 09:42

Update prod.py

cdbe0cd

- don't use get

Update factories.py

b72daf2

SonQBChau merged commit ef4a554 into develop May 23, 2024
6 checks passed

candigbot mentioned this pull request May 23, 2024

CanDIG/katsu merging: DIG-1582: Implement overview thresholds & DIG-1533: Change cancer_type to primary_site CanDIG/CanDIGv2#605

Closed

This was referenced May 24, 2024

DIG-1533: cancer_type->primary_site & DIG-1583: Implement censoring on aggregate count threshold CanDIG/candigv2-query#40

Merged

Enable data censoring and rename cancer_type to primary_site CanDIG/CanDIGv2#613

Merged

SonQBChau deleted the sonchau/overview_threshold branch August 14, 2024 22:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DIG-1582: Implement overview thresholds & DIG-1533: Change cancer_type to primary_site #205

DIG-1582: Implement overview thresholds & DIG-1533: Change cancer_type to primary_site #205

SonQBChau commented May 10, 2024 •

edited

Loading

codecov bot commented May 10, 2024 •

edited

Loading

CourtneyGosselin left a comment

OrdiNeu commented May 21, 2024 •

edited

Loading

SonQBChau commented May 21, 2024

SonQBChau commented May 21, 2024

OrdiNeu commented May 21, 2024

SonQBChau commented May 22, 2024 •

edited

Loading

OrdiNeu commented May 22, 2024

SonQBChau commented May 22, 2024

OrdiNeu left a comment •

edited

Loading

OrdiNeu May 23, 2024

SonQBChau May 23, 2024

OrdiNeu May 23, 2024

SonQBChau May 23, 2024

OrdiNeu May 23, 2024

OrdiNeu May 23, 2024

SonQBChau May 23, 2024

OrdiNeu May 23, 2024

SonQBChau May 23, 2024

OrdiNeu commented May 23, 2024 •

edited

Loading

		"""


		class OverviewTestCase(BaseTestCase):

DIG-1582: Implement overview thresholds & DIG-1533: Change cancer_type to primary_site #205

DIG-1582: Implement overview thresholds & DIG-1533: Change cancer_type to primary_site #205

Conversation

SonQBChau commented May 10, 2024 • edited Loading

What's new

Types of Change(s)

Has it been tested for:

How to test

codecov bot commented May 10, 2024 • edited Loading

Codecov Report

CourtneyGosselin left a comment

Choose a reason for hiding this comment

OrdiNeu commented May 21, 2024 • edited Loading

SonQBChau commented May 21, 2024

SonQBChau commented May 21, 2024

OrdiNeu commented May 21, 2024

SonQBChau commented May 22, 2024 • edited Loading

OrdiNeu commented May 22, 2024

SonQBChau commented May 22, 2024

OrdiNeu left a comment • edited Loading

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

OrdiNeu commented May 23, 2024 • edited Loading

SonQBChau commented May 10, 2024 •

edited

Loading

codecov bot commented May 10, 2024 •

edited

Loading

OrdiNeu commented May 21, 2024 •

edited

Loading

SonQBChau commented May 22, 2024 •

edited

Loading

OrdiNeu left a comment •

edited

Loading

OrdiNeu commented May 23, 2024 •

edited

Loading