MyPy, lots of MyPy #123

Lnaden · 2019-08-29T16:33:09Z

This PR makes Elemental pass MyPy. I've done what I can to make not
use the type: ignore comment best I can, but Pydantic and MyPy don't
seem to play nice in many cases.

Outstanding issues I have no good solution to:

Typed Dict not available until 3.8
lambda functions with kwargs that have defaults can't be inffered and Callable does not work.
MyPy does not like the constr from Pydantic
Not sure how to fix the Array type acceptance, if I try to make it have a Generic[T] type, it complains about metaclass types
Not sure if the Array Generic type is really a good idea?
compare_X functions cannot import ProtoModel since it creates a recursive import as other things import from testing.

Next major step would be to include the .pyi stub files so things like Engine can recognize the typed functions

This PR makes Elemental pass MyPy. I've done what I can to make not use the `type: ignore` comment best I can, but Pydantic and MyPy don't seem to play nice in many cases. Outstanding issues I have no good solution to: * Typed Dict not available until 3.8 * `lambda` functions with kwargs that have defaults can't be inffered and Callable does not work. * MyPy does not like the `constr` from Pydantic * Not sure how to fix the Array type acceptance, if I try to make it have a `Generic[T]` type, it complains about metaclass types * Not sure if the `Array` Generic type is really a good idea? * `compare_X` functions cannot import ProtoModel since it creates a recursive import as other things import from testing.

codecov · 2019-08-29T16:35:54Z

Codecov Report

Merging #123 into master will decrease coverage by 0.05%.
The diff coverage is 93.33%.

lgtm-com · 2019-08-29T17:04:16Z

This pull request introduces 2 alerts when merging efe3e95 into 711f6fe - view on LGTM.com

new alerts:

1 for Unused import
1 for Mismatch between signature and use of an overridden method

loriab

great! (though you're stealing my favorite test bed for learning typing :-) )

loriab · 2019-08-29T16:47:49Z

qcelemental/__init__.py

+periodictable = periodic_table.periodictable
+PhysicalConstantsContext = physical_constants.PhysicalConstantsContext
+constants = physical_constants.constants
+CovalentRadii = covalent_radii.CovalentRadii


why do we need CovaletRadii (the class)?

This __init__ preserves all the same imports as before, just in a way which makes MyPy happy. Now would be a good time to revisit the imports of singletons while we're in here tinkering if something does not belong.

looks like the class is exposed for the tests and docs. and it's exposed so ppl can create a Context, whereas the physconst context gets its own class b/c it has extra pint intricacies.

so this is fine. future vdwRadii and similar can have their contexts exposed through class while physconst stays with own class. so long as predictable.

loriab · 2019-08-29T16:56:32Z

qcelemental/molparse/chgmult.py

        cgmp_rules.append('4-' + str(ifr))

    #   * (R5) require total parity consistent among neutral_electrons, chg, and mult
    cgmp_range.append(lambda c, fc, m, fm: _parity_ok(zel, c, m))
    cgmp_rules.append('5')
    for ifr in range(nfr):
-        cgmp_range.append(lambda c, fc, m, fm, ifr=ifr: _parity_ok(fzel[ifr], fc[ifr], fm[ifr]))
+        cgmp_range.append(lambda c, fc, m, fm, ifr=ifr: _parity_ok(fzel[ifr], fc[ifr], fm[ifr]))  # type: ignore


The ifr=ifr and chg=chg are less a "default" argument than a "capture" argument for lambda instantiation time. But until one can declare for ifr: int in range(nfr):, I can see where typing would have a problem.

For some reason mypy has issues with any of the lambda functions which have a ifr=ifr like construct in its args. It was able to understand the rest of them. I tried doing a Callable[..., bool] and Callable[[float, float, float, float], bool] but it did not like that, nor does the Callable support kwargs in anyway, so capturing the ifr and chg kwargs is impossible with that (and MyPy complains about the signature if you try, and then still cant parse the lambda).

So the "for now" fix was to ignore it until we we engineer it differently.

your sol'n is fine by me. I'm glad to be aware of the problem.

loriab · 2019-08-29T17:00:12Z

qcelemental/molparse/to_string.py

+        def to_dict(self) -> Dict:
+            return {attr: getattr(self, attr) for attr in self._dict_attrs}
+
+    data = Data()


huh, that works, too. too bad can't tie back into models.Molecule, whose fields these are. thanks.

There are a couple ways I found to do this, one is wait until 3.8 and we get the TypedDict class, other places this was a thing I just declared the variable a x: Dict = ... and moved on. This one I tried the dataclass-like construct here since it was fixed keys and entirely internal as it was an ensured way of enforcing the typing.

Since this I did make this a Class, even internal, if its undesired, I can roll it back to just being a declared Dict[Any, Any] and having MyPy effectively ignore it.

nope, it's fine. my comment was conversational.

dgasmith

Not too terrible to do overall?

dgasmith · 2019-08-29T17:13:32Z

qcelemental/physical_constants/context.py

@@ -7,6 +7,8 @@
 from functools import lru_cache
 from typing import Union

+from pint import quantity, UnitRegistry


Cannot import pint here. Far too slow and must be lazily imported.

Maybe similar to: https://github.com/samuelcolvin/pydantic/blob/master/pydantic/main.py#L23

dgasmith · 2019-08-29T17:14:46Z

qcelemental/tests/test_molparse_from_string.py

@@ -379,7 +380,7 @@ def test_psi4_qm_3():
    'fragment_separators': [],
 }

-fullans5b = {'efp': {}}
+fullans5b: Dict = {'efp': {}}


Do we need to type tests? Seems annoying.

Probably not, these were quick though and I was just ticking items off the list. I can mark mypy to ignore all the tests.

Lnaden

Not too terrible to do overall?

The end result is. The amount of tinkering I had to do to learn the oddities (such as just x: Dict =... a variable) took a long time since there is no real solution there, its more of a " just ignore this, MyPy" order. I also spent a good long while looking at the Array[some_type] constructs and in the end could not figure out how to tell MyPy that Array can take a type argument. I can go into more details about what I tried if you want more details though.

Lnaden · 2019-08-29T17:25:32Z

One thing I found was that the testing module is actually imported by other places in the code to gain access to the compare_X routines. What are the thoughts on porting those to the util module so I can try to do proper typing without a recursive import? The issue with that is the testing module then becomes depleted of most its functions.

I could also just leave it and rely on forward ref type hints for free.

loriab · 2019-08-29T17:27:54Z

I'm not keen on emigration from testing b/c I've got several other projects importing qcel.testing for its goodies. If making testing into a directory and separating pieces that way helped typing, I'm fine with that.

Lnaden · 2019-08-29T17:31:59Z

If making testing into a directory and separating pieces that way helped typing, I'm fine with that.

Eh... maybe? It would take a bit of engineering. I'm fine leaving it as is and suspected the testing module was being used elsewhere due to its handy nature, hence why I asked. I'll just leave it as a forward ref.

lgtm-com · 2019-08-29T18:14:08Z

This pull request introduces 3 alerts when merging 0b1fc37 into 711f6fe - view on LGTM.com

new alerts:

2 for Unused import
1 for Mismatch between signature and use of an overridden method

lgtm-com · 2019-08-29T18:49:10Z

This pull request introduces 2 alerts when merging a4750a9 into 711f6fe - view on LGTM.com

new alerts:

1 for Unused import
1 for Mismatch between signature and use of an overridden method

Lnaden requested review from dgasmith and loriab August 29, 2019 16:33

Cleanup some unused imports

efe3e95

loriab approved these changes Aug 29, 2019

View reviewed changes

dgasmith reviewed Aug 29, 2019

View reviewed changes

Lnaden commented Aug 29, 2019

View reviewed changes

Update from comments

0b1fc37

LGTM cleanup

a4750a9

Lnaden merged commit 7cb0fc8 into MolSSI:master Aug 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MyPy, lots of MyPy #123

MyPy, lots of MyPy #123

Lnaden commented Aug 29, 2019

codecov bot commented Aug 29, 2019 •

edited

Loading

lgtm-com bot commented Aug 29, 2019

loriab left a comment

loriab Aug 29, 2019

Lnaden Aug 29, 2019

loriab Aug 29, 2019

loriab Aug 29, 2019

Lnaden Aug 29, 2019 •

edited

Loading

loriab Aug 29, 2019

loriab Aug 29, 2019

Lnaden Aug 29, 2019

loriab Aug 29, 2019

dgasmith left a comment

dgasmith Aug 29, 2019

dgasmith Aug 29, 2019

dgasmith Aug 29, 2019

Lnaden Aug 29, 2019

Lnaden left a comment

Lnaden commented Aug 29, 2019

loriab commented Aug 29, 2019

Lnaden commented Aug 29, 2019

lgtm-com bot commented Aug 29, 2019

lgtm-com bot commented Aug 29, 2019

MyPy, lots of MyPy #123

MyPy, lots of MyPy #123

Conversation

Lnaden commented Aug 29, 2019

codecov bot commented Aug 29, 2019 • edited Loading

Codecov Report

lgtm-com bot commented Aug 29, 2019

loriab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lnaden Aug 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgasmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lnaden left a comment

Choose a reason for hiding this comment

Lnaden commented Aug 29, 2019

loriab commented Aug 29, 2019

Lnaden commented Aug 29, 2019

lgtm-com bot commented Aug 29, 2019

lgtm-com bot commented Aug 29, 2019

codecov bot commented Aug 29, 2019 •

edited

Loading

Lnaden Aug 29, 2019 •

edited

Loading