[Refactor] Clarify a few csv_importer types and names

[dshemetov]

A few small refactors for clarity that I couldn't help myself in making while working on #947.

Most of the changes are purely for better readability. No functionality is altered, except for the interface of load_csv, but this should not affect actual usage.

Changes:

• rename RowValues to CsvRowValue
• make CsvRowValue into a dataclass
• remove pandas optional argument from load_csv that was only used for test mocks
• use unittest.patch to mock test instead
• update other tests
• update csv_to_database with a few relative imports

Further details:

I changed

# From
def load_csv(filepath, geo_type, pandas=pandas):

# To
def load_csv(filepath, geo_type):

where pandas was just an abstract argument to the Pandas library meant for mocking. Instead of complicating this function's interface, we can just use @patch("pandas.read_csv") in the tests, which automatically mocks any calls to that function with our desired mock object.

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

[dshemetov]

Problem

CI is failing. I can repro on my local machine by running the same command CI runs

docker run -i --rm --network delphi-net \
		--env "SQLALCHEMY_DATABASE_URI=mysql+mysqldb://user:pass@delphi_database_epidata:3306/epidata" \
		--env "FLASK_SECRET=abc" \
		delphi_web_python python -m pytest --import-mode importlib repos/delphi/delphi-epidata/tests/

wherein it complains about the relative imports I use

_ ERROR collecting repos/delphi/delphi-epidata/tests/acquisition/covidcast/test_csv_importer.py _
ImportError while importing test module '/usr/src/app/repos/delphi/delphi-epidata/tests/acquisition/covidcast/test_csv_importer.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
repos/delphi/delphi-epidata/tests/acquisition/covidcast/test_csv_importer.py:16: in <module>
    from ....src.acquisition.covidcast.csv_importer import CsvImporter, CsvRowValue
E   ModuleNotFoundError: No module named 'repos.delphi.delphi-epidata.src'

But I didn't run into this issue locally when running the same command with a volume mount

docker run -i --rm --network delphi-net \
		--mount type=bind,source=$(CWD)repos/delphi/delphi-epidata,target=/usr/src/app/repos/delphi/delphi-epidata,readonly \
		--mount type=bind,source=$(CWD)repos/delphi/delphi-epidata/src,target=/usr/src/app/delphi/epidata,readonly \
		--env "SQLALCHEMY_DATABASE_URI=mysql+mysqldb://user:pass@delphi_database_epidata:3306/epidata" \
		--env "FLASK_SECRET=abc" \
		delphi_web_python python -m pytest --import-mode importlib repos/delphi/delphi-epidata/tests/

How does the volume mount remove this issue? 🤔 It's not changing the paths at all, only updating the contents.

---

Answer

UPDATE: so it appears that we're adding a new source directory with the volume mount that is usually not present.

# CI Version
> docker run -i --rm -t delphi_web_python ls repos/delphi/delphi-epidata/
Jenkinsfile  deploy.json  integrations       package.json          scripts
LICENSE      dev          labels             pyproject.toml        tasks.py
README.md    devops       mypy.ini           requirements.dev.txt  testdata
build.js     docs         package-lock.json  requirements.txt      tests

# Mount Version
> docker run -i --rm --network delphi-net \
                --mount type=bind,source=/home/dskel/Documents/Code/Delphi/delphi-epidata-dev/driver/repos/delphi/delphi-epidata,target=/usr/src/app/repos/delphi/delphi-epidata,readonly \
                --mount type=bind,source=/home/dskel/Documents/Code/Delphi/delphi-epidata-dev/driver/repos/delphi/delphi-epidata/src,target=/usr/src/app/delphi/epidata,readonly \
                -t delphi_web_python ls repos/delphi/delphi-epidata/
Jenkinsfile  dev           mypy.ini              requirements.txt  tests
LICENSE      devops        package-lock.json     scripts
README.md    docs          package.json          src
build.js     integrations  pyproject.toml        tasks.py
deploy.json  labels        requirements.dev.txt  testdata

So in the CI version, the src directory is not there, which is what my relative imports rely on.

Future Work

Can we just add that directory to our Docker image instructions? Looking at the CI file, we can see that the setup.sh script here actually moves the src directories from all the repos over. We could fix this by changing the mv command to a cp command.

I'm going to leave this alone for now. There are many ways to "fix" our directory issues there and I'm not ready to do an exhaustive review of all our options for fixing that at this point (maybe relative imports, maybe directory restructuring, maybe renaming delphi-epidata to something without a hypen, etc.).

Resolution

So I'm just going to let this go, focus on the JIT meta computation priority, and revert all the relative directory imports.

[krivard]

Gorgeous change, I'll accept the long line lengths as a necessary evil if I must

[krivard]

🏆

[krivard]

I find this harder to read but I will hold my nose and accept it as the way of the future

[krivard]

Ugh same, I don't like it but it's fine

[dshemetov]

I guess I have to admit it... I'm a long-line lover. There are dozens of us. Dozens!