Skip to content
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.

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

Update to mypy 0.9 #638

Merged
merged 6 commits into from
Apr 26, 2022
Merged

Update to mypy 0.9 #638

merged 6 commits into from
Apr 26, 2022

Conversation

ascillitoe
Copy link
Contributor

@ascillitoe ascillitoe commented Apr 25, 2022

Update to mypy 0.900. This is a precursor to #605.

This PR is similar to SeldonIO/alibi-detect#490, but with a few extra changes:

  • In explainers/cfproto.py two lines that previously raised type-var arguments are now raising arg-type errors, so the type: ignore's have been updated to ignore these. arg-type errors seem logical to me here, but I might be missing something.

  • The second call to ray.remote() below:

    DistributedAnchorTabular.ray.remote(RemoteSampler).remote(
                        *(train_data_id, d_train_data_id, sampler)

    results in the error:

    No overload variant of "remote" of "RemoteFunction" matches argument type "Tuple[Any, Any, TabularSampler]"
    

    This doesn't seem surprising since the overloaded type sigs are all similar to:

    def remote(self, arg0: Optional[ObjectRef[None]])
    

    i.e. ObjectRef's are always expected, rather than Any. I don't have an answer for why mypy<0.9 doesn't pick this up though... To avoid an error, type: ignore[call-overload] is added.

@ascillitoe
Copy link
Contributor Author

ascillitoe commented Apr 25, 2022

@jklaise @mauicv any thoughts on the new type: ignore's detailed above?

@ascillitoe ascillitoe added this to the 0.6.6 milestone Apr 25, 2022
@codecov
Copy link

codecov bot commented Apr 25, 2022

Codecov Report

Merging #638 (cce2108) into master (27706c7) will decrease coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #638      +/-   ##
==========================================
- Coverage   82.06%   81.96%   -0.10%     
==========================================
  Files          77       77              
  Lines       10519    10519              
==========================================
- Hits         8632     8622      -10     
- Misses       1887     1897      +10     
Impacted Files Coverage Δ
alibi/explainers/anchor_tabular.py 78.72% <ø> (ø)
alibi/explainers/cfproto.py 75.53% <100.00%> (ø)
alibi/explainers/anchor_base.py 84.81% <0.00%> (-2.54%) ⬇️
alibi/explainers/ale.py 59.79% <0.00%> (-0.51%) ⬇️
alibi/utils/distributed.py 67.11% <0.00%> (-0.45%) ⬇️

@ascillitoe ascillitoe mentioned this pull request Apr 26, 2022
Copy link
Contributor

@jklaise jklaise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I would wager that the type-var to arg-type error was a bug in the older version of mypy.

@ascillitoe
Copy link
Contributor Author

I would wager that the type-var to arg-type error was a bug in the older version of mypy.

Yeh agreed, arg-type does make more sense for those lines.

@ascillitoe ascillitoe merged commit 7effb5f into SeldonIO:master Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants