Bump pre-commit versions + add isort

[illusional]

Happy new time to bump versions of our pre-commits - it's truly the most magical time of the year.

This time, a new friend isort. It's mostly enabled in vscode now, already in our pyproject.toml, so time to make things official.

[milo-hyben]

Looking good to me.

[jmarshall]

By default, isort will edit files in place. As a pre-commit hook, it will print a message, abort the commit, and invite you to look at what it has done to your files — which may be mixed up with your own unstaged changes.

IMHO it would be better for it to do what at least several of the other hooks do: print detailed messages about its complaints and abort the commit, but not affect the files. So I would add:

   hooks:
     - id: isort
       name: isort (python)
+      args: [--check, --diff, --color]
+      additional_dependencies: [colorama]

[jmarshall]

We also have a problem that we believe in more categories of imports than isort does (by default). For example, our production-pipelines code has:

import hailtop.batch as hb
from hailtop.batch.job import Job

from cpg_utils import Path
from cpg_utils.config import get_config

because hail is externally-supplied while cpg_utils is CPG-local, and we consider those to be separate categories.

OTOH isort will move from hailtop.batch.job import Job to below the other from … import lines and remove the blank line because it considers them to be all the same category.

We may be able to add --thirdparty/--project/etc settings to args: or pyproject.toml to teach it to agree with us.

[illusional]

Black, prettier and our own custom pre-commit hooks (https://github.com/populationgenomics/cpg-infrastructure-private/blob/c97c31f9d1fe626e1831c9fbc066a529449837e2/sortyamlfiles.py#L113-L114) modifies files in place too, FWIW this is generally how I use pre-commit to fix auto fixable errors.

If still not sure, maybe it's best to wait for the software team meeting to chat about it there.

[illusional]

@jmarshall, what do you think of Forced Separate:

[tool.isort]
py_version = 311
line_length = 88
sections = ['FUTURE', 'STDLIB', 'HAIL', 'CPG', 'THIRDPARTY', 'FIRSTPARTY', 'LOCALFOLDER']
known_hail = [
    "hail",
    "hailctl",
]
known_cpg = [
    "analysis-runner",
    "cpg_utils",
    "cpg_infra",
    "cpg_workflows",
    "metamist",    
]

[jmarshall]

I think that's pretty good, and I'm pleased isort is this configurable. Running it on the production-pipelines repo suggests some tweaks to adjust it to our existing conventions a bit more and also shows that it will probably need a bit of per-repository tweaking:

• The PEP8 convention is stdlib / thirdparty / local. Our existing convention (at least in prod-pipes) has hail and CPG in between thirdparty and local. So I'd suggest reordering to

sections = ['FUTURE', 'STDLIB', 'THIRDPARTY', 'HAIL', 'CPG', 'FIRSTPARTY', 'LOCALFOLDER']

• known_hail should be hail and hailtop. Maybe hailctl too if it is used somewhere, but we certainly need to add hailtop.

• Should we prevent isort from editing submodules? Do we tell other linters to ignore submodules? For prod-pipes for example this would be

extend_skip = ['gnomad_methods', 'seqr-loading-pipelines']

Per-repository tweaking:

• For production-pipelines, we would want to omit cpg_workflows from "known_cpg" so that it would be considered local, and add "gnomad" and "hail_scripts" as they come from gnomad_methods (which is either CPG or maybe local as it is a submodule though not fully qualified…). This may well vary for each repository.

[illusional]

Thanks @jmarshall, those suggestions sound great - especially avoiding submodules! Do you mind adding a commit to this branch with your suggestions, maybe a line in the TOML that suggests you might want to tweak these on a per-repo basis?