Skip to content

Address general type issues #1922

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Address general type issues #1922

wants to merge 7 commits into from

Conversation

@victorlin
Copy link
Member

@victorlin victorlin commented Oct 27, 2025 •
edited
Loading

Description of proposed changes

Enable reportGeneralTypeIssues and address existing issues. Commits roughly ordered from simplest to most complex.

Related issue(s)

Follow-up to:

augur/pyrightconfig.json

Lines 14 to 15 in e19f2c2

// TODO: go through these exceptions and determine if they should be kept or
// removed and violations addressed.

#1917 (review) suggested adding type hints, and I figured this PR would make the hints more useful.

Checklist

  • Automated checks pass
  • Check if you need to add a changelog message
  • Check if you need to add tests
  • Check if you need to update docs

@victorlin
Report general type issues
b3c9ac6 
From doc¹:

    Generate or suppress diagnostics for general type inconsistencies,
    unsupported operations, argument/parameter mismatches, etc. This covers
    all of the basic type-checking rules not covered by other rules. It does
    not include syntax errors.

Existing issues will be addressed in subsequent commits.

¹ https://microsoft.github.io/pyright/#/configuration?id=type-check-rule-overrides
@victorlin
Raise ValueError instead of string
1f90db2 
Raising a string literal instead of a proper exception is not allowed by
Pyright.
@victorlin
Use Python 3.9 compatible Optional[str]
f94b6a8 
Union syntax is only supported by Python ≥3.10.
@victorlin
Define specific dict types
43ab838 
This allows Pyright to know the dict structure for type checking.
@victorlin
Add helper function for new AttrInfo object
525ff6d 
This removes duplicated lambdas and the need for type annotation on the
'seen' variable.
@victorlin
Replace iterrows() with itertuples()
1535943 
There was a type error with the previous code:

    subsample.py:353:12 - error: Invalid conditional operand of type "Series[bool]"
        Method __bool__ for type "Series[bool]" returns type "NoReturn" rather than "bool" (reportGeneralTypeIssues)

This is because iterrows() yields Series, and Series.__getitem__ returns
Series[Any] | Any (scalar). Pyright sees row[…] as that type, which
means the outcome of the comparison operator is Series[bool] | bool.
Series[bool] cannot be used as-is for the if condition.

This could be addressed by casting 'int(…) < int(…)', but switching to
itertuples() seems more appropriate as it addresses the type error and
is more efficient.¹

The internal column names have been stripped of their leading
underscores to avoid being renamed.²

¹ https://stackoverflow.com/a/70227993
² "Notes" section in https://pandas.pydata.org/pandas-docs/version/2.3/reference/api/pandas.DataFrame.itertuples.html
@victorlin victorlin self-assigned this Oct 27, 2025
@victorlin
Copy link
Member Author

victorlin commented Oct 27, 2025

This introduced new Mypy errors:

augur/validate_export.py:55: error: Need type annotation for "seen"  [var-annotated]
augur/validate_export.py:204: error: Need type annotation for "seen"  [var-annotated]
…
augur/filter/subsample.py:353: error: "str" not callable  [operator]
augur/filter/subsample.py:353: error: "bytes" not callable  [operator]
augur/filter/subsample.py:353: error: "date" not callable  [operator]
augur/filter/subsample.py:353: error: "datetime" not callable  [operator]
augur/filter/subsample.py:353: error: "timedelta" not callable  [operator]
augur/filter/subsample.py:353: error: "datetime64[Union[date, int]]" not callable  [operator]
augur/filter/subsample.py:353: error: "timedelta64[Union[timedelta, int]]" not callable  [operator]
augur/filter/subsample.py:353: error: "bool" not callable  [operator]
augur/filter/subsample.py:353: error: "int" not callable  [operator]
augur/filter/subsample.py:353: error: "float" not callable  [operator]
augur/filter/subsample.py:353: error: "Timestamp" not callable  [operator]
augur/filter/subsample.py:353: error: "Timedelta" not callable  [operator]
augur/filter/subsample.py:353: error: "complex" not callable  [operator]
augur/filter/subsample.py:353: error: "integer[Any]" not callable  [operator]
augur/filter/subsample.py:353: error: "floating[Any]" not callable  [operator]
augur/filter/subsample.py:353: error: "complexfloating[Any, Any]" not callable  [operator]
Found 18 errors in 2 files (checked 82 source files)

The first 2 aren't necessary (Pyright infers that seen is ~TreeAttrs) and all the rest are false positives (Pyright infers that row is _PandasNamedTuple, not any of the types reported by Mypy).

For now, I'll plan to add # type: ignore to appease Mypy. Long-term, with a more comprehensive Pyright config, it might be good to ditch Mypy to align with nextstrain/cli@4a4e500.

@victorlin
Ignore new Mypy errors
eca7aaa 
Errors for augur/validate_export.py:

    Need type annotation for "seen"  [var-annotated]

This is unnecessary – Pyright correctly infers that seen is ~TreeAttrs.

Errors for augur/filter/subsample.py:

    "str" not callable  [operator]
    "bytes" not callable  [operator]
    "date" not callable  [operator]
    …

These are false positives – Pyright correctly infers that row is
_PandasNamedTuple, not any of the types reported by Mypy.
huddlej
huddlej reviewed Oct 27, 2025
View reviewed changes
@codecov
Copy link

codecov bot commented Oct 27, 2025

Codecov Report

❌ Patch coverage is 90.90909% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.18%. Comparing base (e19f2c2) to head (eca7aaa).

Files with missing lines Patch % Lines
augur/dates/ambiguous_date.py 0.00% 1 Missing ⚠️
augur/validate_export.py 92.30% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1922      +/-   ##
==========================================
+ Coverage   74.15%   74.18%   +0.02%     
==========================================
  Files          82       82              
  Lines        8986     8996      +10     
  Branches     1828     1828              
==========================================
+ Hits         6664     6674      +10     
  Misses       2018     2018              
  Partials      304      304

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@victorlin victorlin mentioned this pull request Oct 27, 2025
Draft
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@huddlej huddlej huddlej left review comments

Assignees

@victorlin victorlin

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@victorlin @huddlej