Flake & Make

[tzaffi]

Adding Makefile and flake8

• Linting related changes:
  *.flake8:
  • Based on my experience in graviton and a couple of extra ignores for formatting that was conflicting with black
  • replace from pyteal import * with import pyteal as pt in our tests, and for other files ignore the * import error on a per file basis (this is the reason line changes number in the thousands which amounts to almost 25% of the entire codebase)
  • Incorporate Remove redundant test imports #277
• Additional changes:
  • Removing requirements.txt (but keeping docs/requirements.txt)
  • setup.py: extras_require={"development" ... replaces the former requirements.txt
  • .github/workflows/build.yml + Makefile: unify as much of the logic as possible. Needed regular python instead of python-slim in order to have make. black is applied to all python files, as before, and flake8 is as well. mypy is applied to scripts in addition to pyteal (in upcoming Run blackbox tests in CI #260 this will be expanded to tests as well).
  • README.md - applying mardkownlint (without automation)

[tzaffi]

@algoidurovic - pulling up the optimizer module. Any objections?

[tzaffi]

resolving as discussed offline

[jasonpaulos]

I think this should remain a submodule. In the future we will likely have more optimizations and there's no reason to clutter the compiler module with them, since their purpose is more specific

[tzaffi]

👍

[michaeldiamant]

@tzaffi For https://www.flake8rules.com/rules/F405.html and https://www.flake8rules.com/rules/F403.html, I'm imagining we'd prefer to avoid the violation in source code. Does it work for you to limit ignoring F403 + F405 to *_test.py and examples/?

From the previous attempt, there's an example ignoring *_test.py: https://github.com/algorand/pyteal/pull/213/files#diff-6951dbb399883798a226c1fb496fdb4183b1ab48865e75eddecf6ceb6cf46442R11.

Running locally, I see there's a F403 violation in source code. I didn't look closely enough to determine how easily it can be avoided vs adding inline ignore. I can take a deeper look if you'd like - let me know.

~/dev/pyteal make-and-flake *1 !1 ?6 ────────────────────────────────────────────────────────────────────────── ✘ 2|1 5s  pyteal 09:53:33 AM
❯ make flake8 | grep -v -E 'test|examples'
pyteal/__init__.py:1:1: F403 'from .ast import *' used; unable to detect undefined names
pyteal/__init__.py:3:1: F403 'from .ir import *' used; unable to detect undefined names
make: *** [flake8] Error 1

[tzaffi]

I'll research this some more and update here.

[tzaffi]

When I first joined I didn't like our profligate use of * imports in pyteal. But the more I use pyteal, the more this convention makes sense to me. We have so many constructs that are typically used in pyteal, that a standard pyteal program would have 10 or more objects being imported.

An alternate approach in such situations (where a library with many constructs is used) is to abbreviate the imported module and then always refer to it. For example:

import pandas as pd

...
df = pd.DataFrame(...)

we could follow a similar convention and start enforcing it here with usages such as below (notice, we are already implicitly recommending this approach with abi):

import pyteal as pt
from pyteal import abi

toSum = abi.DynamicArray(abi.Uint64TypeSpec())
toMultiply = abi.DynamicArray(abi.Uint64TypeSpec())
results = abi.Tuple(abi.Uint64TypeSpec(), abi.Uint64TypeSpec())
program = pt.Seq(
    toSum.decode(pt.Txn.application_args[0]),
    toMultiply.decode(pt.Txn.application_args[1]),
    sumAndMultiply(toSum, toMultiply).store_into(results)
)

[michaeldiamant]

A few thoughts from my side:

• If folks are on-board, I am on-board to use the pandas as pd approach. I think it'd help disambiguate PyTeal constructs that match Python standard library (e.g. Array).
• If folks prefer to keep as is, I'd advocate to add the ignore only for tests and examples. I'm thinking in source code we can be more perspective. Though I'll ultimately follow the group's recommendation here.

[jasonpaulos]

There's 3 separate issues:

• Use of star imports in our runtime code: this is highly useful because we have a few modules that import and export all of a submodule. It would be too time consuming and error prone to not use a star import here. My opinion is that adding inline exceptions for these cases is good enough.
• Use of star imports in example: from the beginning we've encouraged this because PyTeal programs can use a lot of different AST elements, and because our syntax is already clunky enough without added prefixed in my opinion. I'm in favor of keeping it as-is since our examples should look natural and not be intimidating.
• Use of star imports in our tests: this matters least to me, so if you'd like to adopt import pyteal as pt or similar, I wouldn't mind.

[tzaffi]

I'll go with the modifying the tests files only and adding flake8 exceptions for other files.

[tzaffi]

hehe... that resulted in 6000+ changes

[michaeldiamant]

@tzaffi Thanks for the changes here - I think it's reasonable starting point and we can iterate over time. Resolving.

[jasonpaulos]

Why $(ALLPY) and not .?

[tzaffi]

It was going into my virutal env directory and failing because of that, so I wanted to explicitly define what is being checked. It's also handy to use this for the other linters (flake8 and eventually mypy). So I would prefer to keep this way.

Another approach is to specifically exclude certain folders with --exclude, but then everyone would have to configure that specifically for their local excludes.

But admittedly, the downside is that if you add another top-level directory, we wouldn't be formatting it by default.

I don't feel strongly about this. LMK

[jasonpaulos]

There's 3 separate issues:

• Use of star imports in our runtime code: this is highly useful because we have a few modules that import and export all of a submodule. It would be too time consuming and error prone to not use a star import here. My opinion is that adding inline exceptions for these cases is good enough.
• Use of star imports in example: from the beginning we've encouraged this because PyTeal programs can use a lot of different AST elements, and because our syntax is already clunky enough without added prefixed in my opinion. I'm in favor of keeping it as-is since our examples should look natural and not be intimidating.
• Use of star imports in our tests: this matters least to me, so if you'd like to adopt import pyteal as pt or similar, I wouldn't mind.

[michaeldiamant]

@tzaffi Thanks for the effort to make the repo more robust. I think it's a good starting point and and we can customize config as we get experience.

[ahangsu]

generally looking good, just one question