Changes to test.sh and pyproject.toml.

[carlosgmartin]

Cleanup related to #1091. Changes include the following:

• Move configuration for pylint, pytest, and pytype to pyproject.toml, a central configuration file.
• Move configuration for flake8 to .flake8. (Flake8 is the odd one out because they reject supporting flake8.)
• Add test_venv/ and optax-*.whl to .gitignore.
• Reorganize test.sh and add progress messages to it.

[fabianp]

thanks!

[vroulet]

Thank you again @carlosgmartin !
I'm leaving you a small comment. I'm really not an expert here, so you and Fabian will know better, but I'm just curious about some pylint errors you disabled.

Also (probably in another PR), one should make different actions in the github workflow to speed up the tests in github (like one for the actual tests, one for pytype, one for pylint, one for the docs). But this can be done in another CL. (right now we are reaching a test time of 19min which is quite long).

[vroulet]

Just to understand: why did you disable all these errors?

Just for you to know: each time a PR does not comply to pylint correctly, we have to patch it internally which slows down the process of migration. So as annoying as pylint may be, we have no other choice to comply with it normally.

I agree that actually the code should be fully cleaned to comply with the usual formattings. If that's the reason for you to disable all these messages let me know. If it's because you are using flake8, that's great.

[carlosgmartin]

@vroulet I disabled these Pylint messages because they were giving me too many errors throughout the repo.

In my latest commit, I was able to re-enable several of them after making many small changes throughout the repo to conform to them.

On my tour across the repo, I noticed that the code formatting is quite inconsistent and not always ideal. I highly recommend adopting a code auto-formatter like black (and maybe isort). For example, in my own personal monorepo, I use the following configuration.

pyproject.toml:

[project.optional-dependencies]
dev = [
    "isort",
    "black",
    "flake8",
    "flake8-comprehensions",
    "pyright",
    "pytest",
]

[tool.pyright]
useLibraryCodeForTypes = false
reportPrivateImportUsage = false
reportIncompatibleMethodOverride = false

[tool.black]
line-length = 79

[tool.isort]
profile = "black"
combine_as_imports = true
line_length = 79

.flake8:

[flake8]
extend-ignore = E203,E741

While developing, I simply run the command

isort . && black . && flake8 && pyright && pytest

If you want, I can make a separate PR that adds black (and maybe isort) and applies it once to the entire repo.

[vroulet]

Hello @carlosgmartin,

Sorry for the delay. I made a pass on the codebase to format all files with our formatter and harmonize the docstrings, see #1129. I am not sure that our internal formatting tool matches black. I don't think so, given that I usually see discrepancies between the formatting you use and ours. So we'll need to fix this later. That pass removed some of the linting issues that were lingering through the codebase.

I see that you made additional changes throughout the codebase. Sorry for the duplicate effort but our internal integration tool sometimes does not accept some external formatting rules, so I preferred to start with them. Looking at your modifications, I think they are orthogonal to some of the modifications I've done so after a merge I think you could also add them.

After that we probably need to add flake8 in the list of files synced between github and the internal version to let this PR pass. I'll take a look once you merged your changes and we will hopefully be able to merge your effort.

By the way, I suppose you wanted to tackle parallel github actions in a separate cl? Our tests on github are rather slow now and the best thing to do would probably be to have separate actions for pytest, pylint, real tests, docs.

Thank you again @carlosgmartin for all your work on this repo! Let me know if you want help for the merge etc...

[carlosgmartin]

@vroulet Merged.

A few suggestions for the future:

• Drop the mixed 2-space/4-space indentation convention, which is inconvenient for development. I'd settle on 4 spaces, which makes it easier to see indentation differences at a glance.
• Expose the code auto-formatter to contributors, so they can apply it when submitting PRs.
• Adopt pre-commit (like JAX) and add the code auto-formatter to it, so it gets applied automatically.

By the way, I suppose you wanted to tackle parallel github actions in a separate cl?

Yes, I'd make that a separate PR. This one's already quite big.

[vroulet]

Hello @carlosgmartin,
Let's break this CL in two: keep the whole pass you did on the code, make a separate pr for fixing the formatter and tests etc... As you pointed out, we should migrate to the same format as what jax/flax uses and in particular use ruff (internally it seems to pyink so I need to ask why is there a difference). And I'd like to merge the whole pass you did before we have to make multiple merges (btw you'll need a merge to fix the broken test here).

A few remarks on your pass:

• let's keep imports in alphabetical order, the exception is that native libraries must be before non-native python libraries.
• the pylint: dissable=g-.... are needed internally generally so please do not remove them.

About the two-space indent policy, it's a choice that Google tool before PEP8. But Google still complies with C0330 for some reason. I/We cannot change this.

[rdyro]

I agree with @vroulet, it'd be good to split the PR into (1) the really good improvements to test.sh and (2) repository linting/tooling.

Perhaps we could move to using ruff for both formatting and linting like in jax? @vroulet @fabianp

[fabianp]

I don't have experience with ruff, but I think it's always a good idea to do as closely as JAX. So +1 for moving to ruff

[fabianp]

@rdyro: did you want me to review these changes or to answer the question? it was unclear to me

[rdyro]

@rdyro: did you want me to review these changes or to answer the question? it was unclear to me

Sorry, I misclicked and didn't see an option to undo it

[vroulet]

Perhaps we could move to using ruff for both formatting and linting like in jax? @vroulet @fabianp

Sounds great. I don't have much experience either with ruff but it would probably be the right move.