Add type hints and apply mypy fix for function attributes where needed #350

LegrandNico · 2021-03-26T22:32:48Z

This PR adds various type hints to increase type coverage. It also fixes the mypy errors raised by the use of function attributes, using the workaround described here.

It will contribute to #200.

Several of the remaining errors returned by mypy are related to inconsistent type inherited from core classes (e.g Variable) and I think this is the next thing to be fixed if we want to solve the rest. I saw what is proposed in #134, but I think I will need a bit more guidance for the next step.

LegrandNico · 2021-03-26T23:28:31Z

This will require adding the typing_extensions at least to the CI dependencies, but I don't know where this should be declared exactly.

brandonwillard · 2021-03-26T23:48:37Z

This will require adding the typing_extensions at least to the CI dependencies, but I don't know where this should be declared exactly.

You can add it to install_requires in setup.py.

codecov · 2021-04-05T12:00:23Z

Codecov Report

Merging #350 (342c960) into master (1366221) will increase coverage by 0.06%.
The diff coverage is 99.63%.

@@            Coverage Diff             @@
##           master     #350      +/-   ##
==========================================
+ Coverage   72.47%   72.53%   +0.06%     
==========================================
  Files         173      173              
  Lines       55631    55775     +144     
==========================================
+ Hits        40316    40459     +143     
- Misses      15315    15316       +1

Impacted Files	Coverage Δ
aesara/link/c/lazylinker_c.py	`76.92% <ø> (ø)`
aesara/graph/callcache.py	`27.27% <83.33%> (+2.27%)`	⬆️
aesara/_version.py	`44.20% <100.00%> (ø)`
aesara/compile/compilelock.py	`97.14% <100.00%> (-2.86%)`	⬇️
aesara/compile/function/types.py	`84.91% <100.00%> (+0.10%)`	⬆️
aesara/compile/io.py	`94.44% <100.00%> (+0.10%)`	⬆️
aesara/compile/sharedvalue.py	`82.05% <100.00%> (+1.76%)`	⬆️
aesara/configparser.py	`90.03% <100.00%> (-0.04%)`	⬇️
aesara/gpuarray/cudnn_defs.py	`37.95% <100.00%> (+0.45%)`	⬆️
aesara/gpuarray/dnn.py	`24.84% <100.00%> (+1.05%)`	⬆️
... and 21 more

LegrandNico · 2021-04-17T17:06:11Z

The code coverage is slightly reduced, and the tests are therefore not completed, but this is due to the lines added after the if TYPE_CHECKING condition, which are not executed by the tests but by mypy directly.

brandonwillard · 2021-04-19T20:36:11Z

The code coverage is slightly reduced, and the tests are therefore not completed, but this is due to the lines added after the if TYPE_CHECKING condition, which are not executed by the tests but by mypy directly.

Perhaps we can add something like the following to our .coveragerc:

[report]
exclude_lines = 
    if TYPE_CHECKING:

michaelosthege · 2021-04-25T07:57:07Z

@LegrandNico can you rebase the branch to resolve conflicts?
If there is an easy fix to exclude those TYPE_CHECKING blocks from coverage calculation let's do it, but before anyone invests more than a few minutes there, we should just not worry about it. After all we know where the decrease comes from.

eigenfoo

Great work! I'm not too familiar with the Protocol decorator workaround, but otherwise this PR looks good to me - I have some minor nitpicky comments, but since all tests pass I think this PR is good to go.

eigenfoo · 2021-05-01T00:03:04Z

aesara/graph/callcache.py


-    def persist(self, filename=None):
+    def persist(self, filename: Optional[str] = None) -> NoReturn:


Shouldn't this be -> None?

The NoReturn return type is for functions that never return normally, not functions that just don't return a value [1] [2].

[1] https://mypy.readthedocs.io/en/stable/more_types.html#the-noreturn-type
[2] https://www.python.org/dev/peps/pep-0484/#the-noreturn-type

I have changed this to -> None.

eigenfoo · 2021-05-01T00:03:27Z

aesara/graph/callcache.py

@@ -46,7 +47,7 @@ def call(self, fn, args=(), key=None):
            _logger.debug("cache hit %i", len(self.cache))
        return self.cache[key]

-    def __del__(self):
+    def __del__(self) -> NoReturn:


Similarly here with NoReturn.

eigenfoo · 2021-05-01T00:05:36Z

aesara/tensor/basic.py

@@ -305,6 +313,8 @@ def get_scalar_constant_value(

    Parameters
    ----------
+    orig_v :


Missing docstring? We should at least add the type here.

I have added this:

""" orig_v : int, float, np.ndarray, Constant or None The variable `v` where to find the constant scalar(0-D). """

aesara/compile/function/__init__.py

eigenfoo · 2021-05-01T00:21:38Z

aesara/tensor/var.py

-    _is_nonzero = True
+    _is_nonzero: bool = True
+    real: Optional[property] = None
+    imag: Optional[property] = None


Not sure what this is - this looks like new code?

If I remember correctly, I had to add imag and real so https://github.com/pymc-devs/aesara/blob/25162ed0f8949aa2509e0b1a04ddfe71b03c9fdb/aesara/tensor/math.py#L1410 does not return an error.

aesara/tensor/var.py

M#

michaelosthege

This PR brings significant improvements of signatures in many places. I rebased it so we can get this going again 🚀

In one file I found type hints that I think are too restrictive.

michaelosthege · 2021-05-24T20:04:15Z

aesara/compile/compilelock.py

@@ -45,15 +45,15 @@ def force_unlock(lock_dir: os.PathLike):


 @contextmanager
-def lock_ctx(lock_dir: os.PathLike = None, *, timeout: Optional[float] = None):
+def lock_ctx(lock_dir: Optional[os.PathLike] = None, *, timeout: Optional[float] = -1):


Suggested change

def lock_ctx(lock_dir: Optional[os.PathLike] = None, *, timeout: Optional[float] = -1):

def lock_ctx(lock_dir: Optional[os.PathLike] = None, *, timeout: Optional[float] = None):

It was intentionally None, because this way it defaults to the config setting.

michaelosthege · 2021-05-24T20:06:28Z

aesara/compile/function/__init__.py

-    on_unused_input=None,
+    inputs: "List[Variable]",
+    outputs: "Optional[List[Variable]]" = None,
+    mode: Union[str] = None,


Suggested change

mode: Union[str] = None,

mode: Union[str, "Mode"] = None,

And further above in the TYPE_CHECKING block from aesara.compile.mode import Mode

Shouldn't the type-hint include Optional?

michaelosthege · 2021-05-24T20:15:34Z

aesara/tensor/basic.py

@@ -1079,7 +1098,7 @@ def grad(self, inp, grads):
 _nonzero = Nonzero()


-def nonzero(a, return_matrix=False):
+def nonzero(a: np.array, return_matrix: bool = False) -> Tuple:


All these functions can take not only numpy arrays, but also TensorVariables or in some cases lists.
The type hint from line 1042 is probably a good candidate for most functions in this file.

LegrandNico · 2021-06-14T23:43:01Z

I accidentally closed this one by renaming the branch master to main, is there a way to reopen it?

brandonwillard · 2021-06-15T02:37:14Z

I accidentally closed this one by renaming the branch master to main, is there a way to reopen it?

Looks like this branch itself was deleted because it was your master branch, so it can't be reopened. You can always create a new PR, but, in the future, be sure to create new branches for all of your work.

michaelosthege requested a review from eigenfoo April 28, 2021 15:07

eigenfoo previously approved these changes May 1, 2021

View reviewed changes

LegrandNico dismissed eigenfoo’s stale review via 5f7fd26 May 8, 2021 10:18

LegrandNico added 6 commits May 24, 2021 21:59

Add type hints and apply mypy fix for function attributes where needed

c7d3731

M#

Add typing_extensions to requirements

0f9871c

Use forward reference for Op in Apply class

27ded22

Resolving conflicts

fe304e6

Remove TYPE_CHECKING lines from code coverage

9b4aa82

Small fixes after PR review

342c960

michaelosthege suggested changes May 24, 2021

View reviewed changes

LegrandNico closed this Jun 14, 2021

LegrandNico deleted the master branch June 14, 2021 21:55

brandonwillard added the typing Work related to type checking label Apr 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type hints and apply mypy fix for function attributes where needed #350

Add type hints and apply mypy fix for function attributes where needed #350

LegrandNico commented Mar 26, 2021

LegrandNico commented Mar 26, 2021

brandonwillard commented Mar 26, 2021

codecov bot commented Apr 5, 2021 •

edited

Loading

LegrandNico commented Apr 17, 2021

brandonwillard commented Apr 19, 2021

michaelosthege commented Apr 25, 2021

eigenfoo left a comment

eigenfoo May 1, 2021

LegrandNico May 8, 2021

eigenfoo May 1, 2021

LegrandNico May 8, 2021

eigenfoo May 1, 2021

LegrandNico May 8, 2021

eigenfoo May 1, 2021

LegrandNico May 8, 2021

michaelosthege left a comment

michaelosthege May 24, 2021

ricardoV94 May 27, 2021

michaelosthege May 24, 2021

ricardoV94 May 27, 2021 •

edited

Loading

michaelosthege May 24, 2021

ricardoV94 May 27, 2021

LegrandNico commented Jun 14, 2021

brandonwillard commented Jun 15, 2021


		def persist(self, filename=None):
		def persist(self, filename: Optional[str] = None) -> NoReturn:

	def lock_ctx(lock_dir: Optional[os.PathLike] = None, *, timeout: Optional[float] = -1):
	def lock_ctx(lock_dir: Optional[os.PathLike] = None, *, timeout: Optional[float] = None):

Add type hints and apply mypy fix for function attributes where needed #350

Add type hints and apply mypy fix for function attributes where needed #350

Conversation

LegrandNico commented Mar 26, 2021

LegrandNico commented Mar 26, 2021

brandonwillard commented Mar 26, 2021

codecov bot commented Apr 5, 2021 • edited Loading

Codecov Report

LegrandNico commented Apr 17, 2021

brandonwillard commented Apr 19, 2021

michaelosthege commented Apr 25, 2021

eigenfoo 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

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

michaelosthege 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

ricardoV94 May 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LegrandNico commented Jun 14, 2021

brandonwillard commented Jun 15, 2021

codecov bot commented Apr 5, 2021 •

edited

Loading

ricardoV94 May 27, 2021 •

edited

Loading