Add `CovidcastRow` testing util and a few other changes #1044

dshemetov · 2022-12-05T21:05:47Z

I made a number of QoL changes to the tests and the server in the process of working on JIT (see also #949). I decided to separate them out, so we can release them separately from JIT and help debug if anything goes wrong.

Built on #1072.

Prerequisites:

Unless it is a documentation hotfix it should be merged against the dev branch
Branch is up-to-date with the branch to be merged with, i.e. dev
Build is successful
Code is cleaned up and formatted

Summary

Adds CovidcastRow testing dataclass, which serves as an abstraction for a row in the database
- Supports many constructors (from arguments, from dictionaries)
- Brings many defaults, so tests only need to specify the fields they care about
- Has a list class companion CovidcastRows that helps in making a bundle of rows
- Supports three views as a DataFrame, with consistently enforced data types for each column
Integrates this class into tests across the codebase
Integrates this class into database.py in acquisition
- Need to update CovidcastRow to remove default values for safety
Adds more_itertools dependency, as it's needed in a few tests and needed later in JIT
Adds PANDAS_DTYPES to model.py to synchronize the datatypes used by Pandas in JIT and in CovidcastRow (will remove, if JIT does not end up using Pandas)
Removes some unused imports in a few server files
Fixes a Pandas warning due to assigning on a view in test_csv_uploading

krivard · 2022-12-08T16:05:15Z

Oh no, #1043 was squash-merged, so now git won't be able to automatically identify any commits from this PR that were also in that one. You'll need to rebase -i and drop the following:

Squash merging is great if you want to quickly tidy away details of the development process that are no longer relevant, but if you're still working on the branch (like this, with several PRs open that chain off one another), they make a mess. Squash with caution!

dshemetov · 2022-12-08T20:08:01Z

True, fortunately I'm ok with rebasing and force pushing into my branches, since no one else develops on these.

krivard

Partial review pass up through ~half of covidcast_row.py

We should probably talk in a synchronous format about how to architect CovidcastRow for use in production vs tests; this complete overlap approach makes me hella nervous

integrations/server/test_covidcast_endpoints.py

src/acquisition/covidcast/covidcast_row.py

integrations/client/test_delphi_epidata.py

integrations/server/test_covidcast.py

integrations/server/test_covidcast_endpoints.py

melange396 · 2023-01-23T20:02:54Z

looking at this now... do you wanna base it against dev? that way you might be able to keep the diff on #1072 smaller

melange396

this is fantastic! it needed to happen and thank you for taking care of it. CovidcastRow came out nice and OO, i really like it.

but im sorry :( there are a whopping 35 comments on this.... a lot are real easy: 3 are just notes you can dismiss, and 17 are nits (almost all w/ pre-populated suggestions). i jumped around a lot while reviewing (not in file-order), but hopefully it should all make sense. the meatier things are: the schema of columns in CovidcastRow, things that may or may not belong in/with the testing base class, semantics around handling of some values, and some readability stuff. i also went a little crazy with the underscores in the date ints.

i am also very happy to get on a zoom to pair up to discuss any or all of my comments.

i must say for the record that i dont love the type annotations everywhere, especially because we are not doing any real rigorous checking or enforcement of them, but i will assume your IDE is telling you that they all check out... i feel a bit like its a plague of locusts coming to descend upon me and maybe i cant get away from it

src/acquisition/covidcast/database.py

integrations/acquisition/covidcast/test_db.py

src/acquisition/covidcast/covidcast_row.py

src/server/endpoints/covidcast_utils/model.py

src/acquisition/covidcast/test_utils.py

integrations/server/test_covidcast_endpoints.py

tests/acquisition/covidcast/test_covidcast_row.py

krivard

Most of these are tagging places where we could drop in as_api_compatibility_dict for as_dict, but I also have a concern about signing future-us up for more test revision hell the next time we do a major schema change

integrations/client/test_delphi_epidata.py

integrations/server/test_covidcast.py

krivard · 2023-01-23T16:24:37Z

tests/acquisition/covidcast/test_covidcast_row.py

+            "source": "src",
+            "signal": "sig",
+            "time_type": "day",
+            "geo_type": "county",
+            "time_value": 20200202,
+            "geo_value": "01234",
+            "value": 5.0,
+            "stderr": 10.0,
+            "sample_size": 10.0,
+            "missing_value": Nans.NOT_MISSING,
+            "missing_stderr": Nans.NOT_MISSING,
+            "missing_sample_size": Nans.NOT_MISSING,
+            "issue": 20200202,
+            "lag": 0,
+            "direction": None


concern: tedious consequences

see "tedious consequences" above -- if any of our defaults ever change, we'll need to update them in 2 places.

So I've addressed this by writing a single expected_df variable at the top of the class and having all the test cases draw from this example.

tests/acquisition/covidcast/test_covidcast_row.py

integrations/server/test_covidcast.py

src/acquisition/covidcast/covidcast_row.py

Co-authored-by: Katie Mazaitis <[email protected]> Co-authored-by: melange396 <[email protected]>

* no default values * helper functions for creating rows

Co-authored-by: Katie Mazaitis <[email protected]> Co-authored-by: melange396 <[email protected]>

dshemetov · 2023-02-05T01:06:16Z

Thanks for all the review comments! I believe I have addressed all of them and synced with @melange396 on it yesterday. I rebased and force pushed to clean up the review commit log. Should be good to merge.

krivard

Glorious triumph 💯

dshemetov requested review from melange396 and krivard December 5, 2022 21:06

dshemetov mentioned this pull request Dec 5, 2022

Bump Pandas to 1.5.1 #1045

Open

5 tasks

dshemetov force-pushed the ds/merge-docker-web-python branch 2 times, most recently from 08b4589 to 8da9b0f Compare December 5, 2022 23:47

dshemetov force-pushed the ds/covidcast-row branch 2 times, most recently from ec5c6c6 to 163185f Compare December 6, 2022 00:33

krivard requested a review from rzats December 6, 2022 15:13

Base automatically changed from ds/merge-docker-web-python to dev December 6, 2022 19:52

dshemetov force-pushed the ds/covidcast-row branch from 163185f to 9fc5535 Compare December 8, 2022 20:09

krivard reviewed Dec 8, 2022

View reviewed changes

dshemetov force-pushed the ds/covidcast-row branch from 8a75d92 to aa25eee Compare January 21, 2023 13:43

dshemetov changed the base branch from dev to ds/refactor-csv-importer January 21, 2023 13:43

dshemetov force-pushed the ds/covidcast-row branch 3 times, most recently from 3bcc0cb to 703d57c Compare January 21, 2023 14:32

Base automatically changed from ds/refactor-csv-importer to dev January 27, 2023 20:49

melange396 reviewed Feb 3, 2023

View reviewed changes

krivard reviewed Feb 3, 2023

View reviewed changes

melange396 mentioned this pull request Feb 3, 2023

make a "src/common/" directory (package?) to share between server and acquisition code #1037

Closed

dshemetov added a commit that referenced this pull request Feb 3, 2023

CovidcastRow: address code review #1044

4f39a82

Co-authored-by: Katie Mazaitis <[email protected]> Co-authored-by: melange396 <[email protected]>

dshemetov force-pushed the ds/covidcast-row branch from ab7607f to 4b54dd6 Compare February 3, 2023 20:29

dshemetov added 5 commits February 4, 2023 16:51

Server: add CovidcastRow helper class for testing

c5c1989

* no default values * helper functions for creating rows

Server: update test_db to use CovidcastRow

694479a

Server: update test_delete_batch to use CovidcastRow

30dd70c

Server: update test_delphi_epidata to use CovidcastRow

45e48ba

Server: update test_covidcast_endpoints to use CovidcastRow

6042524

dshemetov and others added 12 commits February 4, 2023 16:51

Server: update test_covidcast to use CovidcastRow

fca788b

Server: update test_utils to use CovidcastRow

79e664b

Server: update TimePair to auto-sort tuples

8ea4018

Server: minor model.py data_source_by_id name update

5400ece

Server: update csv issue none handling

01b225e

Server: add type hints to _query

7be40fa

Acquisition: update test_csv_uploading to remove Pandas warning

91427cd

Server: add PANDAS_DTYPES to model.py

c101a3c

Docker: add more_itertools==8.4.0 to Python and API images

3e214e2

Acquisition: update database.py to use CovidcastRow

7268fc0

Acquisition: update csv_importer to use CovidcastRow

4178862

CovidcastRow: address code review #1044

b3cba4c

Co-authored-by: Katie Mazaitis <[email protected]> Co-authored-by: melange396 <[email protected]>

dshemetov force-pushed the ds/covidcast-row branch from 183187a to b3cba4c Compare February 5, 2023 00:56

krivard approved these changes Feb 6, 2023

View reviewed changes

krivard merged commit 766959b into dev Feb 6, 2023

krivard deleted the ds/covidcast-row branch February 6, 2023 15:32

krivard mentioned this pull request Feb 9, 2023

Release Delphi Epidata 0.4.6 #1081

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `CovidcastRow` testing util and a few other changes #1044

Add `CovidcastRow` testing util and a few other changes #1044

dshemetov commented Dec 5, 2022 •

edited

Loading

krivard commented Dec 8, 2022 •

edited

Loading

dshemetov commented Dec 8, 2022

krivard left a comment

melange396 commented Jan 23, 2023

melange396 left a comment

krivard left a comment

krivard Jan 23, 2023

dshemetov Feb 3, 2023

dshemetov commented Feb 5, 2023

krivard left a comment

Add CovidcastRow testing util and a few other changes #1044

Add CovidcastRow testing util and a few other changes #1044

Conversation

dshemetov commented Dec 5, 2022 • edited Loading

Summary

krivard commented Dec 8, 2022 • edited Loading

dshemetov commented Dec 8, 2022

krivard left a comment

Choose a reason for hiding this comment

melange396 commented Jan 23, 2023

melange396 left a comment

Choose a reason for hiding this comment

krivard left a comment

Choose a reason for hiding this comment

krivard Jan 23, 2023

Choose a reason for hiding this comment

dshemetov Feb 3, 2023

Choose a reason for hiding this comment

dshemetov commented Feb 5, 2023

krivard left a comment

Choose a reason for hiding this comment

Add `CovidcastRow` testing util and a few other changes #1044

Add `CovidcastRow` testing util and a few other changes #1044

dshemetov commented Dec 5, 2022 •

edited

Loading

krivard commented Dec 8, 2022 •

edited

Loading