PyBaMM
PyBaMM copied to clipboard
Published
20 hours ago •
pybamm-team
pybamm-team
Check adherence with `sp-repo-review` guidelines
There are a lot of interesting things available in the Scientific Python ecosystem, perhaps we should also try using sp-repo-review?
Originally posted by @agriyakhetarpal in https://github.com/pybamm-team/PyBaMM/issues/3390#issuecomment-1786984884
Output from running sp-repo-review locally
General:
• Detected build backend: MISSING
├── PY001 Has a pyproject.toml ❌
│ All projects should have a pyproject.toml file to support a modern build system and support wheel installs properly.
├── PY002 Has a README.(md|rst) file ✅
├── PY003 Has a LICENSE* file ✅
├── PY004 Has docs folder ✅
├── PY005 Has tests folder ✅
├── PY006 Has pre-commit config ✅
└── PY007 Supports an easy task runner (nox or tox) ✅
PyProject:
├── PP002 Has a proper build-system table [skipped]
├── PP003 Does not list wheel as a build-dep [skipped]
├── PP301 Has pytest in pyproject [skipped]
├── PP302 Sets a minimum pytest to at least 6 [skipped]
├── PP303 Sets the test paths [skipped]
├── PP304 Sets the log level in pytest [skipped]
├── PP305 Specifies xfail_strict [skipped]
├── PP306 Specifies strict config [skipped]
├── PP307 Specifies strict markers [skipped]
├── PP308 Specifies useful pytest summary [skipped]
└── PP309 Filter warnings specified [skipped]
GitHub Actions:
├── GH100 Has GitHub Actions config ✅
├── GH101 Has nice names ✅
├── GH102 Auto-cancel on repeated PRs ✅
├── GH103 At least one workflow with manual dispatch trigger ✅
├── GH200 Maintained by Dependabot ✅
├── GH210 Maintains the GitHub action versions with Dependabot ✅
└── GH211 Do not pin core actions as major versions ✅
Pre-commit:
├── PC100 Has pre-commit-hooks ✅
├── PC110 Uses black or ruff-format ❌
│ Must have https://github.com/psf/black-pre-commit-mirror or https://github.com/astral-sh/ruff-pre-commit + id: ruff-format in
│ .pre-commit-config.yaml
├── PC111 Uses blacken-docs [skipped]
├── PC140 Uses mypy ❌
│ Must have https://github.com/pre-commit/mirrors-mypy repo in .pre-commit-config.yaml
├── PC160 Uses codespell ❌
│ Must have https://github.com/codespell-project/codespell repo in .pre-commit-config.yaml
├── PC170 Uses PyGrep hooks (only needed if RST present) ✅
├── PC180 Uses prettier ❌
│ Must have https://github.com/pre-commit/mirrors-prettier repo in .pre-commit-config.yaml
├── PC190 Uses Ruff ✅
├── PC191 Ruff show fixes if fixes enabled ❌
│ If --fix is present, --show-fixes must be too.
└── PC901 Custom pre-commit CI message ✅
MyPy:
├── MY100 Uses MyPy (pyproject config) [skipped]
├── MY101 MyPy strict mode [skipped]
├── MY102 MyPy show error codes [skipped]
├── MY103 MyPy warn unreachable [skipped]
├── MY104 MyPy enables ignore-without-code [skipped]
├── MY105 MyPy enables redundant-expr [skipped]
└── MY106 MyPy enables truthy-bool [skipped]
Ruff:
├── RF001 Has Ruff config [skipped]
├── RF002 Target version must be set [skipped]
├── RF003 src directory specified if used [skipped]
├── RF101 Bugbear must be selected [skipped]
├── RF102 isort must be selected [skipped]
├── RF103 pyupgrade must be selected [skipped]
├── RF201 Avoid using deprecated config settings [skipped]
└── RF202 Use (new) lint config section [skipped]
Documentation:
├── RTD100 Uses ReadTheDocs (pyproject config) ✅
├── RTD101 You have to set the RTD version number to 2 ✅
├── RTD102 You have to set the RTD build image ✅
└── RTD103 You have to set the RTD python version ✅
The build-system check will be fixed when #3301 is merged and related checks that are skipped can be checked again. @Saransh-cpp reports that we can skip using isort, codespell, and related tools in https://github.com/pybamm-team/PyBaMM/issues/3390#issuecomment-1787103645. Type checkers like MyPy can wait and can be revisited once type hints are added for PyBaMM models. For ruff-related checks, we have #3477 which a PR for can tackle.
Here's an updated output from sp-repo-review:
Expand to view
General:
• Detected build backend: setuptools.build_meta
• Detected license(s): BSD License
├── PY001 Has a pyproject.toml ✅
├── PY002 Has a README.(md|rst) file ✅
├── PY003 Has a LICENSE* file ✅
├── PY004 Has docs folder ✅
├── PY005 Has tests folder ✅
├── PY006 Has pre-commit config ✅
└── PY007 Supports an easy task runner (nox or tox) ✅
PyProject:
├── PP002 Has a proper build-system table ✅
├── PP301 Has pytest in pyproject ✅
├── PP302 Sets a minimum pytest to at least 6 ✅
├── PP303 Sets the test paths ✅
├── PP304 Sets the log level in pytest ✅
├── PP305 Specifies xfail_strict ✅
├── PP306 Specifies strict config ✅
├── PP307 Specifies strict markers ✅
├── PP308 Specifies useful pytest summary ✅
└── PP309 Filter warnings specified ✅
GitHub Actions:
├── GH100 Has GitHub Actions config ✅
├── GH101 Has nice names ✅
├── GH102 Auto-cancel on repeated PRs ✅
├── GH103 At least one workflow with manual dispatch trigger ✅
├── GH104 Use unique names for upload-artifact ❌
│ Multiple upload-artifact usages must have unique names to be compatible with v4 (which no longer merges artifacts, but instead errors out). The most general
│ solution is:
│
│
│ - uses: actions/upload-artifact@v4
│ with:
│ name: prefix-${{ github.job }}-${{ strategy.job-index }}
│
│ - uses: actions/download-artifact@v4
│ with:
│ pattern: prefix-*
│ merge-multiple: true
│
│
│ • No variable substitutions were detected in publish_pypi.yml:build_macos_and_linux_wheels.
├── GH200 Maintained by Dependabot ✅
├── GH210 Maintains the GitHub action versions with Dependabot ✅
├── GH211 Do not pin core actions as major versions ✅
└── GH212 Require GHA update grouping ✅
Pre-commit:
├── PC100 Has pre-commit-hooks ✅
├── PC110 Uses black or ruff-format ✅
├── PC111 Uses blacken-docs ✅
├── PC140 Uses mypy ❌
│ Must have https://github.com/pre-commit/mirrors-mypy repo in .pre-commit-config.yaml
├── PC160 Uses codespell ❌
│ Must have https://github.com/codespell-project/codespell repo in .pre-commit-config.yaml
├── PC170 Uses PyGrep hooks (only needed if RST present) ✅
├── PC180 Uses prettier ❌
│ Must have https://github.com/pre-commit/mirrors-prettier repo in .pre-commit-config.yaml
├── PC190 Uses Ruff ✅
├── PC191 Ruff show fixes if fixes enabled ✅
└── PC901 Custom pre-commit CI message ✅
MyPy:
├── MY100 Uses MyPy (pyproject config) ✅
├── MY101 MyPy strict mode ❌
│ Must have strict in the mypy config. MyPy is best with strict or nearly strict configuration. If you are happy with the strictness of your settings already,
│ ignore this check or set strict = false explicitly.
│
│
│ [tool.mypy]
│ strict = true
│
├── MY102 MyPy show_error_codes deprecated ✅
├── MY103 MyPy warn unreachable ❌
│ Must have warn_unreachable (true/false) to pass this check. There are occasionally false positives (often due to platform or Python version static checks), so
│ it's okay to set it to false if you need to. But try it first - it can catch real bugs too.
│
│
│ [tool.mypy]
│ warn_unreachable = true
│
├── MY104 MyPy enables ignore-without-code ❌
│ Must have "ignore-without-code" in enable_error_code = [...]. This will force all skips in your project to include the error code, which makes them more
│ readable, and avoids skipping something unintended.
│
│
│ [tool.mypy]
│ enable_error_code = ["ignore-without-code", "redundant-expr", "truthy-bool"]
│
├── MY105 MyPy enables redundant-expr ❌
│ Must have "redundant-expr" in enable_error_code = [...]. This helps catch useless lines of code, like checking the same condition twice.
│
│
│ [tool.mypy]
│ enable_error_code = ["ignore-without-code", "redundant-expr", "truthy-bool"]
│
└── MY106 MyPy enables truthy-bool ❌
Must have "truthy-bool" in enable_error_code = []. This catches mistakes in using a value as truthy if it cannot be falsey.
[tool.mypy]
enable_error_code = ["ignore-without-code", "redundant-expr", "truthy-bool"]
Ruff:
├── RF001 Has Ruff config ✅
├── RF002 Target version must be set ✅
├── RF003 src directory specified if used [skipped]
├── RF101 Bugbear must be selected ✅
├── RF102 isort must be selected ❌
│ Must select the isort I checks. Recommended:
│
│
│ [tool.ruff.lint]
│ extend-select = [
│ "I", # isort
│ ]
│
├── RF103 pyupgrade must be selected ✅
├── RF201 Avoid using deprecated config settings ✅
└── RF202 Use (new) lint config section ✅
Documentation:
├── RTD100 Uses ReadTheDocs (pyproject config) ✅
├── RTD101 You have to set the RTD version number to 2 ✅
├── RTD102 You have to set the RTD build image ✅
└── RTD103 You have to set the RTD python version ✅
@Saransh-cpp, it looks like we have a few MyPy-related issues here, could you ensure that they are fixed (or addressed as best they can based on feasibility) when you return to #3853?
P.S. I'll take up the "Have unique names for artifacts" point soon.