MyPy Configuration Issue LSP in profilers

[JGSweets]

Currently, we are avoiding some MyPy errors by ignoring override errors:
https://github.com/capitalone/DataProfiler/blob/main/setup.cfg#L18

Need to address / understand how to address this issue so we can remove the flag.

This is related to LSP. https://en.wikipedia.org/wiki/Liskov_substitution_principle

[junholee6a]

When I remove the disable_error_code = override from setup.cfg and run pre-commit run --all-files, mypy says there are 12 errors, and they're all in dataprofiler/profilers/. But if I run mypy dataprofiler/data_readers, it shows 7 errors in dataprofiler/data_readers/. So is the pre-commit only type checking dataprofiler/profilers/? And is this unintentional?

[JGSweets]

data_readers should be included. I'm not seeing a specific config that would suggest otherwise.

Since data_readers is in a directory, is it possible it is bc of relative paths?

[JGSweets]

@junholee6a maybe provide a sample of the data_reader errors from pre-commit?

[taylorfturner]

Was just typing this haha... @JGSweets beat me to it

[junholee6a]

Output of pre-commit run --all-files:

Output of mypy dataprofiler/data_readers

[taylorfturner]

Interesting... the pre-commit config should only exclude what is here: https://github.com/capitalone/DataProfiler/blob/main/.pre-commit-config.yaml#L40

[junholee6a]

I was able to get this output from pre-commit run --all-files by creating a type error in dataprofiler/data_readers/csv_data.py. The first error is in dataprofiler/data_readers/, so it seems like the pre-commit is checking all of the files as it should. It's probably checking them with a different configuration or dependency list from a basic mypy command though. This might be okay as it is since it seems to catch egregious type errors. Or, maybe there are dependencies missing in .pre-commit-config.yaml (pre-commit runs mypy from an isolated virtualenv, so it doesn't have access to the existing venv)

[junholee6a]

I tried replacing the additional_dependencies list for mypy in .pre-commit-config.yaml with the output of pip freeze, and got the output below from pre-commit run --all-files. The output includes the seven errors in dataprofiler/data_readers/, so the discrepancy between the pre-commit and the mypy command was because they ran with different packages installed. I guess the ideal would be to be running the pre-config with all of the packages in the various requirements....txt files, installed?

[JGSweets]

Interesting! Yes, I think we should be running given the correct package installations.

We're open to you thoughts on how to proceed and resolve these issues.

[junholee6a]

@JGSweets thank you for your receptiveness! Pre-commit doesn't support pulling dependencies from a requirements.txt file, so I was thinking about writing a pre-commit hook that fails if the mypy dependencies in .pre-commit-config.yaml don't match the contents of the requirements...txt files, and adds the dependencies to .pre-commit-config.yaml if needed. That way, the mypy pre-commit dependencies are synced with the requirements...txt files upon committing. Does this seem alright? I'm open to work on a PR.