Fully type-annotate and type-check everything #100

jwodder · 2021-12-06T16:12:34Z

@satra At the moment, the following nontrivial issues are causing problems with the type checking:

The dynamic creation of enum types from dicts does not play well with mypy at all.
mypy does not like these two lines, and I'm not even convinced they work as intended anyway:

https://github.com/dandi/dandischema/blob/62067fbb0184d51f2dd9ee415cf014f74249cd78/dandischema/models.py#L137-L138

The definition of wasGeneratedBy in Dandiset — which declares the field as a Optional[List[Project]] — is incompatible with the definition of the field in the supertype — which declares it as Optional[List[Activity]].
The definition of url in PublishedDandiset — which declares the field as a str — is incompatible with the definition of the field in the supertype — which declares it as Optional[HttpUrl]. If PublishedDandiset.url is changed to an HttpUrl, pydantic then complains about the field's regex parameter not applying.
mypy doesn't like the overriding of HttpUrl based on an environment variable.

codecov · 2021-12-06T16:14:31Z

Codecov Report

Patch coverage: 99.70% and project coverage change: +1.04 🎉

Comparison is base (5b48f26) 96.67% compared to head (0259c29) 97.71%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #100      +/-   ##
==========================================
+ Coverage   96.67%   97.71%   +1.04%     
==========================================
  Files          18       17       -1     
  Lines        1654     1751      +97     
==========================================
+ Hits         1599     1711     +112     
+ Misses         55       40      -15

Flag	Coverage Δ
unittests	`97.71% <99.70%> (+1.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
dandischema/tests/utils.py	`100.00% <ø> (ø)`
dandischema/models.py	`97.69% <99.49%> (+3.42%)`	⬆️
dandischema/consts.py	`100.00% <100.00%> (ø)`
dandischema/datacite.py	`95.45% <100.00%> (ø)`
dandischema/digests/dandietag.py	`95.58% <100.00%> (+0.77%)`	⬆️
dandischema/digests/tests/test_dandietag.py	`100.00% <100.00%> (ø)`
dandischema/digests/tests/test_zarr.py	`100.00% <100.00%> (ø)`
dandischema/digests/zarr.py	`98.73% <100.00%> (ø)`
dandischema/exceptions.py	`100.00% <100.00%> (ø)`
dandischema/metadata.py	`96.63% <100.00%> (ø)`
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

dandischema/digests/dandietag.py

satra

some comments. i'll address your comments in a separate thread.

satra · 2021-12-13T17:14:23Z

dandischema/datacite.py

@@ -134,7 +134,7 @@ def to_datacite(
                NAME_PATTERN, contr_el.name
            ).pop()

-            if getattr(contr_el, "affiliation"):
+            if hasattr(contr_el, "affiliation") and contr_el.affiliation is not None:


can this not be replaced by the following pattern?

Suggested change

if hasattr(contr_el, "affiliation") and contr_el.affiliation is not None:

if getattr(contr_el, "affiliation", None):

If using getattr, mypy can't tell whether contr_el.affiliation is None or not, so it'll warn when it's used in a non-None way inside the block.

that sounds like a deficiency in mypy since logically it cannot be None since then if would not evaluate to True. Do you know of the issue may be already in mypy on this?

python/mypy#11142 seems like a good starting point.

dandischema/exceptions.py

satra · 2021-12-13T17:47:23Z

The dynamic creation of enum types from dicts does not play well with mypy at all.

mypy doesn't tend to like anything dynamic. if you have a suggestion let me know. we need more info around those enums, so turning them into enums to start with will lose that capability.

mypy does not like these two lines, and I'm not even convinced they work as intended anyway:
dandischema/dandischema/models.py

this was added by @yarikoptic instead of manually writing schemaKey in every class. in terms of functionality i believe it works, can you suggested why you think it doesn't work?

The definition of wasGeneratedBy in Dandiset — which declares the field as a Optional[List[Project]] — is incompatible with the definition of the field in the supertype — which declares it as Optional[List[Activity]].

a Project is a subclass of Activity, which should be fine from a class perspective.

The definition of url in PublishedDandiset — which declares the field as a str — is incompatible with the definition of the field in the supertype — which declares it as Optional[HttpUrl]. If PublishedDandiset.url is changed to an HttpUrl, pydantic then complains about the field's regex parameter not applying.

pydantic doesn't allow regex's for url patterns. and we need to be more specific as to what that published url looks like. this is why it was changed to a str.

mypy doesn't like the overriding of HttpUrl based on an environment variable.

i don't have a good suggestion, but we will likely continue with dynamic configurations.

jwodder · 2021-12-13T17:56:53Z

The dynamic creation of enum types from dicts does not play well with mypy at all.

mypy doesn't tend to like anything dynamic. if you have a suggestion let me know. we need more info around those enums, so turning them into enums to start with will lose that capability.

Need more info for what? In the current state, I don't see what is gained from the dynamic construction.

mypy does not like these two lines, and I'm not even convinced they work as intended anyway:
dandischema/dandischema/models.py

this was added by @yarikoptic instead of manually writing schemaKey in every class. in terms of functionality i believe it works, can you suggested why you think it doesn't work?

While the Field assignment works, the Literal annotation does not, as annotating a dict key doesn't do anything; quoting the Python docs: "For expressions as assignment targets, the annotations are evaluated if in class or module scope, but not stored."

The definition of wasGeneratedBy in Dandiset — which declares the field as a Optional[List[Project]] — is incompatible with the definition of the field in the supertype — which declares it as Optional[List[Activity]].

a Project is a subclass of Activity, which should be fine from a class perspective.

And yet mypy complains. I think it has something to do with the covariance/contravariance of List.

The definition of url in PublishedDandiset — which declares the field as a str — is incompatible with the definition of the field in the supertype — which declares it as Optional[HttpUrl]. If PublishedDandiset.url is changed to an HttpUrl, pydantic then complains about the field's regex parameter not applying.

pydantic doesn't allow regex's for url patterns. and we need to be more specific as to what that published url looks like. this is why it was changed to a str.

You could use a validator instead of a regex.

mypy doesn't like the overriding of HttpUrl based on an environment variable.

i don't have a good suggestion, but we will likely continue with dynamic configurations.

What's wrong with just always using AnyHttpUrl?

satra · 2021-12-13T18:05:00Z

Need more info for what

for json-ld graph constructions. the dictionaries from which the enums are built have additional properties around those values.

You could use a validator instead of a regex.

we are validating through multiple routes using both python and json schema validator. regex's also apply to json schema. anyhttpurl is too general and also does not support regex's. the intent here is to be as specific as possible for both routes, and specialize in python only when necessary. regex's therefore support both routes to start with.

jwodder · 2021-12-13T18:48:23Z

@satra

Is anything currently consuming the dicts for JSON-LD purposes? What other ways can the data be provided to such consumers?
Need to provide support for different "instances" of DANDI archive #76 and some other issues I've seen around about supporting alternative URLs imply that you may want to drop the regex restriction on PublishedDandiset.url soon. If that happens, could we just change that field and all other URL fields to plain AnyHttpUrl?

yarikoptic · 2022-03-25T21:18:45Z

should we resurrect this one? seems has aged enough, would need rebase etc...

jwodder · 2022-03-28T16:06:14Z

@yarikoptic I would like to resurrect this, but in order to be worthwhile, we would need to deal with the various issues listed in the first comment. (cc @satra)

satra · 2022-03-30T17:07:48Z

for AnyHttpUrl see this open issue: pydantic/pydantic#3143 (so just shifting to anyhttpurl won't solve the mypy issue). also we do have something specific that makes it httpurl, it seems forcing something to be more general just for static validation seems to be counter to the idea of specificity of elements.

i welcome suggestions, but we should not move away from our overall goals of maintaining both a json schema that can do a fair bit of validating and pydantic, which can add additional validators. the dandi web app uses the json schema for validation, so we can't simply move away from those elements.

jwodder · 2022-03-31T13:13:51Z

I'm not saying we should move away from the JSON Schema, but I would like to know how the current enum metadata is consumed & used so that a static alternative can be devised. The dynamic enum generation is currently the biggest problem with getting mypy to be happy with this library, and I have yet to see a real reason why it's done the way it is.

yarikoptic · 2022-10-06T13:27:55Z

I would like to revive this development so we could get dandischema annotated and then dandi-cli.
It seems that some (not sure if applicable) workaround for pydantic/AnyHttpUrl issue was suggested, see pydantic/pydantic#3143 (comment) - could it be applied here?
@jwodder, would you be kind to merge in master (not sure if comments would survive with rebase)?

jwodder · 2022-10-06T13:30:09Z

@yarikoptic

would you be kind to merge in master

You mean rebase this PR against master, not merge this PR into master, right?

yarikoptic · 2022-10-06T14:53:28Z

@yarikoptic

would you be kind to merge in master

You mean rebase this PR against master, not merge this PR into master, right?

correct. I guess I should have said "merge master in [this branch]" ;-)

jwodder · 2022-10-06T15:46:58Z

@yarikoptic Rebased.

yarikoptic · 2023-02-16T15:59:07Z

@satra - could you please re-review this PR, and follow up on existing discussions. It would be nice to finish up this PR and get it merged so the work doesn't get wasted and we get better type annotations checking through out.

yarikoptic · 2023-02-17T13:54:36Z

Meanwhile: I've rebased and force-pushed to get a fresh state. Locally I get "Found 132 errors in 7 files (checked 21 source files)", so delaying finishing it up causes the growth of annotations to "catch up on". I can promise keeping an eye on this PR to get it over the finish line if @jwodder makes another push to add missing annotations.

dandischema/metadata.py

satra · 2023-02-17T14:12:40Z

@jwodder - there are several intentional tests for checking validation, which the type checker seems to pick up on as invalid.

satra · 2023-03-04T02:03:23Z

fine with me.

yarikoptic · 2023-03-04T14:59:44Z

which one is fine with you - 0.6.4 or 0.7.0 @satra?

also any change in schema version results in dandi-cli testing going kaboom since dandi-archive is not accepting future/unknown version of the schema, so I guess we nohow tune up the docker compose setup while testing dandischema to use this particular dandischema "version" @jwodder ?

satra · 2023-03-04T16:47:17Z

i should have been clearer. for this 0.6.4 is fine. there are probably bigger changes coming, for which we will want a bump to 0.7.0. (btw, dandischema will probably go through a significant overhaul as we align neuro data models across projects. eta: 3 months or so)

Co-authored-by: Satrajit Ghosh <[email protected]>

Because the attempt at setting it dynamically doesn't actually produce any annotations

Looking at the changes exported schema introduces in https://github.com/dandi/dandi-schema/actions/runs/4325493337/jobs/7551722366 I think that they might (since would have needed to look at all uses where Enums were introduced) be compatible. But I can be wrong, time will show ;)

jwodder added the minor Increment the minor version when merged label Dec 6, 2021

jwodder requested a review from satra December 6, 2021 16:12

yarikoptic reviewed Dec 7, 2021

View reviewed changes

dandischema/digests/dandietag.py Show resolved Hide resolved

jwodder mentioned this pull request Dec 7, 2021

Remove dandi/core/ in favor of copy in dandischema dandi/dandi-cli#849

Merged

satra reviewed Dec 13, 2021

View reviewed changes

jwodder mentioned this pull request Mar 2, 2022

Update zarr checksums #120

Merged

jwodder force-pushed the typing branch from 9d86fe1 to 3d05c25 Compare October 6, 2022 15:44

jwodder force-pushed the typing branch from 3d05c25 to 33b1ba6 Compare October 6, 2022 21:13

jwodder force-pushed the typing branch 2 times, most recently from 62a45ff to dd881dd Compare October 14, 2022 17:12

yarikoptic requested a review from satra February 16, 2023 15:59

jwodder mentioned this pull request Feb 16, 2023

Enable Pydantic mypy plugin in type-checking con/solidation#14

Merged

yarikoptic force-pushed the typing branch from 6acf248 to 1ede9d2 Compare February 17, 2023 13:52

satra reviewed Feb 17, 2023

View reviewed changes

dandischema/metadata.py Outdated Show resolved Hide resolved

jwodder and others added 24 commits March 6, 2023 13:05

Type-annotate all the functions

b6348df

Add mypy

ba013ff

Fix a bunch of typing errors (but not all)

0f65c62

Add a py.typed file

e2b3fd4

Add a type-checking GitHub Action

5e8e1ae

Fix some type errors

a56f43c

Fix test failures under pre-Python 3.9

a1357a7

Test DandiETag.get_part_etag()

1c7db2b

Even more type-annotations

5df0025

typing.yml: Update action versions

56761ac

Update dandischema/metadata.py

8820faf

Co-authored-by: Satrajit Ghosh <[email protected]>

Various typing fixes

032ed6b

Unrestrict mypy version

6aed1f0

Run pre-commit on everything

a85c81f

Define enums statically

7bd738b

Add an annotation

a4ebe99

Default DANDI_INSTANCE_URL to localhost; always use AnyHttpUrl

a12f2e4

Hard-code schemaKey for all classes

f032ea5

Because the attempt at setting it dynamically doesn't actually produce any annotations

Expand schemaKeys for parent classes

f224281

Address the last two errors

23e5685

Fix for older Pythons

7b5c4a7

Default DANDI_INSTANCE_URL to None

cd0962f

Don't override locally-built Docker image by pulling from Docker Hub

0259c29

jwodder force-pushed the typing branch from 5143171 to 0259c29 Compare March 6, 2023 18:07

yarikoptic merged commit 2b8fd73 into master Mar 6, 2023

yarikoptic deleted the typing branch March 6, 2023 19:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fully type-annotate and type-check everything #100

Fully type-annotate and type-check everything #100

jwodder commented Dec 6, 2021

codecov bot commented Dec 6, 2021 •

edited

Loading

satra left a comment

satra Dec 13, 2021

jwodder Dec 13, 2021

yarikoptic Mar 3, 2023

jwodder Mar 3, 2023

satra commented Dec 13, 2021

jwodder commented Dec 13, 2021

satra commented Dec 13, 2021

jwodder commented Dec 13, 2021

yarikoptic commented Mar 25, 2022

jwodder commented Mar 28, 2022

satra commented Mar 30, 2022

jwodder commented Mar 31, 2022

yarikoptic commented Oct 6, 2022

jwodder commented Oct 6, 2022

yarikoptic commented Oct 6, 2022

jwodder commented Oct 6, 2022

yarikoptic commented Feb 16, 2023

yarikoptic commented Feb 17, 2023

satra commented Feb 17, 2023

satra commented Mar 4, 2023

yarikoptic commented Mar 4, 2023

satra commented Mar 4, 2023

	if hasattr(contr_el, "affiliation") and contr_el.affiliation is not None:
	if getattr(contr_el, "affiliation", None):

Fully type-annotate and type-check everything #100

Fully type-annotate and type-check everything #100

Conversation

jwodder commented Dec 6, 2021

codecov bot commented Dec 6, 2021 • edited Loading

Codecov Report

satra left a comment

Choose a reason for hiding this comment

satra Dec 13, 2021

Choose a reason for hiding this comment

jwodder Dec 13, 2021

Choose a reason for hiding this comment

yarikoptic Mar 3, 2023

Choose a reason for hiding this comment

jwodder Mar 3, 2023

Choose a reason for hiding this comment

satra commented Dec 13, 2021

jwodder commented Dec 13, 2021

satra commented Dec 13, 2021

jwodder commented Dec 13, 2021

yarikoptic commented Mar 25, 2022

jwodder commented Mar 28, 2022

satra commented Mar 30, 2022

jwodder commented Mar 31, 2022

yarikoptic commented Oct 6, 2022

jwodder commented Oct 6, 2022

yarikoptic commented Oct 6, 2022

jwodder commented Oct 6, 2022

yarikoptic commented Feb 16, 2023

yarikoptic commented Feb 17, 2023

satra commented Feb 17, 2023

satra commented Mar 4, 2023

yarikoptic commented Mar 4, 2023

satra commented Mar 4, 2023

codecov bot commented Dec 6, 2021 •

edited

Loading