Fixes most mypy errors and run it on CI #82

guilhermeleobas · 2023-07-05T18:55:45Z

This PR fixes most mypy errors and enable it on CI.

kc611

Along with the comments above, could you give this PR a run of pre-commit after these changes. (Specifically black recommended some changes)

numba_rvsdg/core/transformations.py

numba_rvsdg/core/datastructures/basic_block.py

.github/workflows/test.yml

guilhermeleobas · 2023-07-10T13:31:33Z

Hi @kc611, thanks for the review. Do you have any other comments or request in this PR?

kc611

Nope, this PR LGTM!

guilhermeleobas · 2023-07-11T18:06:14Z

Thank you

esc · 2023-07-12T08:58:35Z

@guilhermeleobas I gave this another review and noticed that I can't run mypy from the command line, but only via pre-commit:

 💣 zsh» mypy numba_rvsdg
numba_rvsdg/rendering/rendering.py:14: error: Skipping analyzing "graphviz": module is installed, but missing library stubs or py.typed marker
numba_rvsdg/rendering/rendering.py:14: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
Found 1 error in 1 file (checked 10 source files)

What else do I need for this to pass?

esc

I reviewed this and it seems, beyond the annotations there has been some functional changes. Specifically, the following example, which works on main at 93ad199 will raise an exception while trying to render:

from numba_rvsdg.rendering.rendering import render_func

def foo(x):
    c = 0
    for i in range(x):
        c += i
        if c <= 0:
            continue
        else:
            for j in range(c):
                c += j
                if c > 100:
                    break
    return c

render_func(foo)

And the exception is:

Traceback (most recent call last):
  File "/Users/esc/git/numba-rvsdg/example2.py", line 16, in <module>
    render_func(foo)
  File "/Users/esc/git/numba-rvsdg/numba_rvsdg/rendering/rendering.py", line 332, in render_func
    render_flow(ByteFlow.from_bytecode(func))
  File "/Users/esc/git/numba-rvsdg/numba_rvsdg/rendering/rendering.py", line 368, in render_flow
    ByteFlowRenderer().render_byteflow(bflow).view("branch restructured")
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/esc/git/numba-rvsdg/numba_rvsdg/rendering/rendering.py", line 200, in render_byteflow
    self.render_block(self.g, name, block)
  File "/Users/esc/git/numba-rvsdg/numba_rvsdg/rendering/rendering.py", line 75, in render_block
    self.render_region_block(digraph, name, block)
  File "/Users/esc/git/numba-rvsdg/numba_rvsdg/rendering/rendering.py", line 154, in render_region_block
    self.render_block(subg, name, block)
  File "/Users/esc/git/numba-rvsdg/numba_rvsdg/rendering/rendering.py", line 75, in render_block
    self.render_region_block(digraph, name, block)
  File "/Users/esc/git/numba-rvsdg/numba_rvsdg/rendering/rendering.py", line 154, in render_region_block
    self.render_block(subg, name, block)
  File "/Users/esc/git/numba-rvsdg/numba_rvsdg/rendering/rendering.py", line 75, in render_block
    self.render_region_block(digraph, name, block)
  File "/Users/esc/git/numba-rvsdg/numba_rvsdg/rendering/rendering.py", line 154, in render_region_block
    self.render_block(subg, name, block)
  File "/Users/esc/git/numba-rvsdg/numba_rvsdg/rendering/rendering.py", line 75, in render_block
    self.render_region_block(digraph, name, block)
  File "/Users/esc/git/numba-rvsdg/numba_rvsdg/rendering/rendering.py", line 154, in render_region_block
    self.render_block(subg, name, block)
  File "/Users/esc/git/numba-rvsdg/numba_rvsdg/rendering/rendering.py", line 75, in render_block
    self.render_region_block(digraph, name, block)
  File "/Users/esc/git/numba-rvsdg/numba_rvsdg/rendering/rendering.py", line 154, in render_region_block
    self.render_block(subg, name, block)
  File "/Users/esc/git/numba-rvsdg/numba_rvsdg/rendering/rendering.py", line 75, in render_block
    self.render_region_block(digraph, name, block)
  File "/Users/esc/git/numba-rvsdg/numba_rvsdg/rendering/rendering.py", line 154, in render_region_block
    self.render_block(subg, name, block)
  File "/Users/esc/git/numba-rvsdg/numba_rvsdg/rendering/rendering.py", line 75, in render_block
    self.render_region_block(digraph, name, block)
  File "/Users/esc/git/numba-rvsdg/numba_rvsdg/rendering/rendering.py", line 154, in render_region_block
    self.render_block(subg, name, block)
  File "/Users/esc/git/numba-rvsdg/numba_rvsdg/rendering/rendering.py", line 77, in render_block
    raise Exception("unreachable")
Exception: unreachable

numba_rvsdg/core/datastructures/scfg.py

numba_rvsdg/core/transformations.py

numba_rvsdg/networkx_vendored/scc.py

esc · 2023-07-12T09:28:33Z

numba_rvsdg/tests/test_utils.py

@@ -1,3 +1,5 @@
+# mypy: ignore-errors


The tests here seem to be annotated, can we include this file too?

guilhermeleobas · 2023-07-12T15:09:28Z

@guilhermeleobas I gave this another review and noticed that I can't run mypy from the command line, but only via pre-commit:
 💣 zsh» mypy numba_rvsdg
numba_rvsdg/rendering/rendering.py:14: error: Skipping analyzing "graphviz": module is installed, but missing library stubs or py.typed marker
numba_rvsdg/rendering/rendering.py:14: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
Found 1 error in 1 file (checked 10 source files)
What else do I need for this to pass?

~~You need to install three packages: types-filelock types-pyyaml types-setuptools. If you run mypy from pre-commit, it will automatically install those for you~~

Not much we can do here, as graphviz doesn't follow PEP-561: It doesn't ship with a py.typed file. See:
xflr6/graphviz#180

guilhermeleobas · 2023-07-12T16:01:31Z

@esc, I fixed the bug you reported in c1a6177

esc · 2023-07-13T08:26:49Z

@guilhermeleobas I gave this another review and noticed that I can't run mypy from the command line, but only via pre-commit:
 💣 zsh» mypy numba_rvsdg
numba_rvsdg/rendering/rendering.py:14: error: Skipping analyzing "graphviz": module is installed, but missing library stubs or py.typed marker
numba_rvsdg/rendering/rendering.py:14: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
Found 1 error in 1 file (checked 10 source files)
What else do I need for this to pass?
~~You need to install three packages: types-filelock types-pyyaml types-setuptools. If you run mypy from pre-commit, it will automatically install those for you~~

Not much we can do here, as graphviz doesn't follow PEP-561: It doesn't ship with a py.typed file. See: xflr6/graphviz#180

OK. Do you know why running it with pre-commit will make it pass, but using mypy directly will not?

esc · 2023-07-13T08:33:11Z

@esc, I fixed the bug you reported in c1a6177

Looks good, thank you, I can confirm that it works.

kc611 · 2023-07-20T11:35:45Z

Is this PR waiting on anything?

If not, can we go ahead with merging it?

guilhermeleobas · 2023-07-20T17:13:17Z

Is this PR waiting on anything?

Not that I am aware of

guilhermeleobas · 2023-07-21T13:37:47Z

@guilhermeleobas I gave this another review and noticed that I can't run mypy from the command line, but only via pre-commit:
 💣 zsh» mypy numba_rvsdg
numba_rvsdg/rendering/rendering.py:14: error: Skipping analyzing "graphviz": module is installed, but missing library stubs or py.typed marker
numba_rvsdg/rendering/rendering.py:14: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
Found 1 error in 1 file (checked 10 source files)
What else do I need for this to pass?
~~You need to install three packages: types-filelock types-pyyaml types-setuptools. If you run mypy from pre-commit, it will automatically install those for you~~
Not much we can do here, as graphviz doesn't follow PEP-561: It doesn't ship with a py.typed file. See: xflr6/graphviz#180
OK. Do you know why running it with pre-commit will make it pass, but using mypy directly will not?

Perhaps it is related to pypa/pip#9502

kc611

LGTM!

The skipping of graphviz checking by mypy can be made into an separate issue for now. There doesn't seem to be a clear reason as to why it is happening and is anyways beyond the scope of the changes being implemented in this PR.

esc · 2023-08-01T13:43:12Z

LGTM!

The skipping of graphviz checking by mypy can be made into an separate issue for now. There doesn't seem to be a clear reason as to why it is happening and is anyways beyond the scope of the changes being implemented in this PR.

OK!

esc · 2023-08-01T13:43:32Z

@guilhermeleobas thank you for the patch. @kc611 thank you for the review.

guilhermeleobas added 7 commits July 5, 2023 15:40

Fixes most mypy errors and run it on CI

6af5a38

Add new line to mypy.ini and uncomment print stmt

d7efc36

flake8

8b6a399

adjust mypy.ini

7c437a8

install types-PyYAML

86f904a

Remove a.py

782381d

add namespace_packages config option

d514867

guilhermeleobas marked this pull request as ready for review July 5, 2023 19:37

kc611 requested changes Jul 6, 2023

View reviewed changes

numba_rvsdg/core/transformations.py Outdated Show resolved Hide resolved

numba_rvsdg/core/datastructures/basic_block.py Outdated Show resolved Hide resolved

kc611 reviewed Jul 6, 2023

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

guilhermeleobas added 5 commits July 6, 2023 15:59

Use pre-commit hook and update mypy failures with strict=True

c113c0c

run pre-commit

0a16f29

run pre-commit in all files

6f9f81f

Ignore test files

ea7d2c6

fix bug introduced by adding annotation to test file

2cfa451

guilhermeleobas requested a review from kc611 July 6, 2023 21:42

kc611 approved these changes Jul 11, 2023

View reviewed changes

esc requested changes Jul 12, 2023

View reviewed changes

address reviewer comments

c1a6177

Merge remote-tracking branch 'upstream/main' into guilhermeleobas/mypy

f11c828

pre-commit

97bf2b5

kc611 approved these changes Aug 1, 2023

View reviewed changes

esc approved these changes Aug 1, 2023

View reviewed changes

esc merged commit cd671f0 into numba:main Aug 1, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes most mypy errors and run it on CI #82

Fixes most mypy errors and run it on CI #82

guilhermeleobas commented Jul 5, 2023

kc611 left a comment

guilhermeleobas commented Jul 10, 2023

kc611 left a comment •

edited

Loading

guilhermeleobas commented Jul 11, 2023

esc commented Jul 12, 2023

esc left a comment

esc Jul 12, 2023

guilhermeleobas commented Jul 12, 2023 •

edited

Loading

guilhermeleobas commented Jul 12, 2023

esc commented Jul 13, 2023

esc commented Jul 13, 2023

kc611 commented Jul 20, 2023 •

edited

Loading

guilhermeleobas commented Jul 20, 2023

guilhermeleobas commented Jul 21, 2023

kc611 left a comment

esc commented Aug 1, 2023

esc commented Aug 1, 2023

Fixes most mypy errors and run it on CI #82

Fixes most mypy errors and run it on CI #82

Conversation

guilhermeleobas commented Jul 5, 2023

kc611 left a comment

Choose a reason for hiding this comment

guilhermeleobas commented Jul 10, 2023

kc611 left a comment • edited Loading

Choose a reason for hiding this comment

guilhermeleobas commented Jul 11, 2023

esc commented Jul 12, 2023

esc left a comment

Choose a reason for hiding this comment

esc Jul 12, 2023

Choose a reason for hiding this comment

guilhermeleobas commented Jul 12, 2023 • edited Loading

guilhermeleobas commented Jul 12, 2023

esc commented Jul 13, 2023

esc commented Jul 13, 2023

kc611 commented Jul 20, 2023 • edited Loading

guilhermeleobas commented Jul 20, 2023

guilhermeleobas commented Jul 21, 2023

kc611 left a comment

Choose a reason for hiding this comment

esc commented Aug 1, 2023

esc commented Aug 1, 2023

kc611 left a comment •

edited

Loading

guilhermeleobas commented Jul 12, 2023 •

edited

Loading

kc611 commented Jul 20, 2023 •

edited

Loading