[DC-1028] [DC-1155] Add script to remove select sites' EHR data #628

mark-velez · 2020-09-22T23:52:39Z

No description provided.

lrwb-aou

Would appreciate your feedback on some of these comments. We will need to fix the unit test before merging. The other feedback could be addressed now or in a new PR, whichever makes the most sense.

data_steward/tools/purge_hpo_data.py

tests/unit_tests/data_steward/tools/purge_hpo_data_test.py

data_steward/tools/purge_hpo_data.py

lrwb-aou

There is at least one change needed to calculating max_results.

data_steward/tools/purge_hpo_data.py

lrwb-aou

So, the exclude script is good. I think we covered that in the previous review. I'd appreciate talking about the logging changes and your design considerations for them.

data_steward/tools/exclude_site_submission.py

data_steward/utils/bq.py

tests/bq_test_helpers.py

tests/unit_tests/data_steward/utils/pipeline_logging_test.py

data_steward/bq_utils.py

mark-velez · 2020-10-23T22:36:23Z

Thanks @lrwb-aou. Let's discuss logging when I get back on Wed. There's some good feedback in here which I may be able to address by then.

mark-velez · 2020-10-24T00:22:32Z

@lrwb-aou I wanted to get as much done on this as I can since I will be out for a few days. I changed the PR title because the build check was failing. I pushed commits I thought were not too controversial and closed the associated conversations; please unresolve if needed. I plan to cover the remaining items when I get back.

danyel-campbell · 2020-10-26T13:50:20Z

@mark-velez @lrwb-aou I merged the changes to fix the intermittent unit test failure for pipeline_logging_test.py so y'all may want to do a pull if you haven't already. 👍🏾

tests/bq_test_helpers.py

data_steward/utils/pipeline_logging.py

mark-velez · 2020-11-04T17:32:51Z

Hey @lrwb-aou please let me know if there's anything left to address before I rebase and remove obsolete code.

data_steward/utils/pipeline_logging.py

lrwb-aou

I've got some thoughts. Let me know what you think.

data_steward/utils/pipeline_logging.py

lrwb-aou

Very few comments. The three comments are about conventions.

data_steward/utils/pipeline_logging.py

tests/bq_test_helpers.py

lrwb-aou · 2020-11-05T21:14:28Z

tests/bq_test_helpers.py

+    str(datetime.date): 'DATE',
+    str(datetime.datetime): 'TIMESTAMP'
+}
+dict_types = Union[Dict, OrderedDict]


I didn't go through creating a NamedTuple because I like this setup you've got here. So I created this very small patch file. What do you think? I know we could let someone use strings, but if these helper functions were used by an integration test, a string object iterable would blow this stuff up. Because I find it unlikely that we'll have single character column names in an integration test. On that subject, do you intend for these functions to be used by integration and unit tests alike?

non_string_iterables.txt

The patch would permit key_order to be List[tuple] or List[List] whereas it is most likely to be passed as a List[str] (example) as it should represent the keys within each row (type=dict) in the order that the result schema. A value belonging to any immutable (hashable?) type is permitted to be used as a key in a dictionary.

I thought maybe the patch had a typo and non_string_iterable_types should be the container type (?), but my IDE is complaining this is invalid.

There is a lot of debate about how to use Iterable without using a string. I don't think there's been a good answer for how to proceed forward. I'm dropping this.

lrwb-aou

This is good. Just need to remove the dead code.

* MagicMock with spec is sufficient

are retrieved * Add `bq_test_helpers` shared helper functions for BigQuery-related tests

* Unpatch generate_paths

* Set configuration with logging.config.dictConfig * Remove get_logger() * Add configure() which operates similar to logging.basicConfig()

* Remove unused warnings import

mark-velez requested a review from lrwb-aou September 22, 2020 23:52

lrwb-aou suggested changes Sep 23, 2020

View reviewed changes

lrwb-aou suggested changes Oct 6, 2020

View reviewed changes

data_steward/tools/purge_hpo_data.py Outdated Show resolved Hide resolved

data_steward/tools/purge_hpo_data.py Outdated Show resolved Hide resolved

data_steward/tools/purge_hpo_data.py Outdated Show resolved Hide resolved

mark-velez force-pushed the mv/purge_hpo branch 2 times, most recently from c7d1699 to 2ca9c9a Compare October 22, 2020 03:30

mark-velez requested a review from lrwb-aou October 22, 2020 15:38

lrwb-aou reviewed Oct 23, 2020

View reviewed changes

lrwb-aou changed the title ~~[DC-1028] Add script to remove select sites' EHR data~~ [DC-1028 / DC-1155] Add script to remove select sites' EHR data Oct 23, 2020

mark-velez changed the title ~~[DC-1028 / DC-1155] Add script to remove select sites' EHR data~~ [DC-1028] [DC-1155] Add script to remove select sites' EHR data Oct 24, 2020

mark-velez commented Oct 28, 2020

View reviewed changes

tests/bq_test_helpers.py Outdated Show resolved Hide resolved

lrwb-aou reviewed Nov 3, 2020

View reviewed changes

data_steward/utils/pipeline_logging.py Show resolved Hide resolved

data_steward/utils/pipeline_logging.py Outdated Show resolved Hide resolved

mark-velez requested a review from lrwb-aou November 4, 2020 17:31

lrwb-aou reviewed Nov 4, 2020

View reviewed changes

data_steward/utils/pipeline_logging.py Outdated Show resolved Hide resolved

lrwb-aou reviewed Nov 4, 2020

View reviewed changes

mark-velez requested a review from lrwb-aou November 4, 2020 23:30

lrwb-aou reviewed Nov 5, 2020

View reviewed changes

data_steward/utils/pipeline_logging.py Outdated Show resolved Hide resolved

tests/bq_test_helpers.py Show resolved Hide resolved

tests/bq_test_helpers.py Show resolved Hide resolved

lrwb-aou reviewed Nov 5, 2020

View reviewed changes

lrwb-aou approved these changes Nov 5, 2020

View reviewed changes

mark-velez added 8 commits November 6, 2020 15:24

[DC-1028] Add script to remove select sites' EHR data

8209e36

Add comments on purge_hpo_data LIST_TABLES_MAX_RESULTS

a3b49a4

Remove bq client instantiation from unit test (breaks circle)

c04747a

* MagicMock with spec is sufficient

Add note to purge_hpo_data docstring that tables are not dropped

80828c2

Add utils.tools.bq.list_tables which ensures all results

769e202

are retrieved * Add `bq_test_helpers` shared helper functions for BigQuery-related tests

Add deprecation warning to bq_utils.list_tables

55518b5

Use bq.list_tables in purge_hpo_data

87e997e

Add get_logger to utils.pipeline_logging

a78e250

mark-velez added 20 commits November 6, 2020 15:24

Use utils.pipeline_logging.get_logger in purge_hpo_data

a084ea2

Use msg in list comprehension for FakeEmit.messages

6acc75c

Prevent inheriting handlers from ancestors in pipeline_logging

271b7ea

Rename purge_hpo_data to exclude_site_submission

e7c3e9a

Use double quotes for variable doc strings

8826ba1

Use alias in pipeline_logging_test

78b0754

Change arg hpo_ids in exclude_site_submission to required option

8bff7b9

Replace DATETIME with TIMESTAMP in bq_test_helpers type field map

883c78b

Fix unit test for pipeline_logging_test

173dce4

* Unpatch generate_paths

Refactor pipeline_logging and usage in exclude_site_submission

677947b

* Set configuration with logging.config.dictConfig * Remove get_logger() * Add configure() which operates similar to logging.basicConfig()

Add hook to log uncaught exceptions

908ebe4

Remove the short option -i from exclude_site_submission

a89391c

Level docstring is more explicit and only configure() is public

da0882f

Add key_order param to bq_test_helpers.mock_query_result

1ddf5f4

Add add_console_handler param to pipeline_logging.configure()

3ce5eae

Remove redundant test in exclude_site_submission_test

910b5c8

Create config dict within pipeline_logging._get_config

d5f9112

Set add_console_handler default to False in pipeline_logging

a0a1775

Add note on future deprecated annotations in bq_test_helpers

a97081a

* Remove unused warnings import

Remove unused pipeline_logging code

d9b10d4

mark-velez force-pushed the mv/purge_hpo branch from af23466 to d9b10d4 Compare November 6, 2020 20:31

mark-velez merged commit f9d2943 into develop Nov 9, 2020

mark-velez deleted the mv/purge_hpo branch November 9, 2020 14:36

[DC-1028] [DC-1155] Add script to remove select sites' EHR data #628

[DC-1028] [DC-1155] Add script to remove select sites' EHR data #628

Conversation

mark-velez commented Sep 22, 2020

Uh oh!

lrwb-aou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lrwb-aou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lrwb-aou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mark-velez commented Oct 23, 2020

Uh oh!

mark-velez commented Oct 24, 2020

Uh oh!

danyel-campbell commented Oct 26, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mark-velez commented Nov 4, 2020

Uh oh!

Uh oh!

lrwb-aou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lrwb-aou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lrwb-aou Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

mark-velez Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

lrwb-aou Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

lrwb-aou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!