Modernize project infrastructure and switching to hatch #108

agoscinski · 2024-07-19T12:26:13Z

With the modernization of the template I took the chance and switched to hatch which is the recommended tool by PyPA.

Project folder layout

Changed to project folder structure to src layout for import parity.

CONTRIBUTING.md

Added CONTRIBUTING.md that explains how the dev tools work. 90% of the functionalities are setting up the hatch inbuilt commands to work with our settings. Someone who is familiar with hatch does not need to know anything more about the internal project structure. This is different to tox for example where everyone has kind of the same environment names, but the still differ in naming and functionality enough that you need to re-adapt a bit. However, for most users hatch is a new tool, therefore I wrote down the most important commands so people can quickly adapt to the new tool.

Building system and publishing

The building system has been changed from flit to hatch. With hatch, building and publishing packages is also very easy. Please read the CONTRIBUTING.md how to build and publish. This is the only thing I have not yet tested myself locally.

Testing

Still using pytest. Updated Python version matrix to aiida-core one (Python 3.9 to 3.12). Updated dependencies to the same as in aiida-core. The only extension was to add ipdb as default debugger for autocompletion. Please read the CONTRIBUTING.md how to run tests.

Static code analysis (Formatting, linting, type checks)

I switched to ruff as formatter and linter as we use it in also aiida-core. Copied settings from aiida-core. Please read the CONTRIBUTING.md how to run these .

pre-commit now only runs hatch-fmt.

Static type checker

mypy is run in a separate environment, since it needs project dependencies that the formatter and linter do not need. Again copied settings from aiida-core except the ones that were marked with # these options reduce the strictness and should eventually be removed. I am not yet sure if we should include it in the CI. I think it is kind of an advanced tool and the plugin developer can choose if he/she want to include it. Also the mypy checks fail and need to be fixed^^ Removed this for now to simplify the PR, will do another PR for it.

Building the docs

Small updates to work with src layout. Usage to build doc can be seen in CONTRIBUTING.md

CI

Updated GitHub actions to the same version as aiida-core and switched to hatch commands. Added a test run to the cookiecutter CI.

update-template-formatter.sh

This is a script initiates a new repo using the template with unique labels, applies the formatter, gets the git diff, replaces the unique labels with the cookie cutter identifier, then applies to on the template. It is not very robust and will fail for certain cases, but it can already safe a lot of work.

TODOs

Resolve aiida deprecation warnings when running pytest --> put into an issue Resolve aiida deprecation warning in the tests #111
Would be nice if we can actually fix the linter and formatting issues in the template and not as hook after the repo is created. --> see update-template-formatter.sh
At the moment the tox.ini is not used. Good practice would be to move the commands in the cookiecutter CI to the tox.ini, and execute tox in the CI. --> not so important so put this into an issue Move CI code into tox so we can run CI locally #110
Fix failing MyPy checks --> will do this in another PR Add static type checker #109

Demo

I put in my fork as default branch so people can try it out with

cookiecutter https://github.com/agoscinski/aiida-plugin-cutter.git

agoscinski · 2024-07-19T16:24:48Z

Maybe one of the reviewer knows a fix for the sphinx build error

/home/alexgo/code/aiida-plugin-cutter/aiida-diff/src/aiida_diff/data/__init__.py:do cstring of aiida_diff.data.DiffParameters.fields:1: WARNING: py:class reference tar get not found: QbFields

It is even more annoying than regular sphinx errors as it vanishes with the second run :)

chrisjsewell · 2024-07-19T19:24:23Z

{{cookiecutter.plugin_name}}/.pre-commit-config.yaml

@@ -1,39 +1,8 @@
-# Install pre-commit hooks via:


Of you are not going to use pre-commit properly, then just remove it entirely

Some people (less on the developer side) are quite annoyed by the formatting as they always forget to apply it and then it makes the CI fail, so I included it in case people still want formatting to happen automatically. The formatting should be fast enough so it does effect commit speed. Also some people might be still used to manually run pre-commit for formatting, so these people can also keep using it.

chrisjsewell · 2024-07-19T19:25:47Z

{{cookiecutter.plugin_name}}/pyproject.toml

+line-length = 120
+
+[tool.ruff.format]
+quote-style = 'single'


I agree, this requires justification. It is an odd change to a perfectly sane default.

I only copied the config from aiida-core. I don't have any opinions on this. I removed it now, since the repo was differently structured and it creates a lot of noise in the changes.

chrisjsewell · 2024-07-19T19:25:55Z

{{cookiecutter.plugin_name}}/pyproject.toml

+source = ["src/{{cookiecutter.module_name}}"]
+
+[tool.ruff]
+line-length = 120


For reference, this replaces the following previous configuration:

[tool.pylint.format] max-line-length = 125 # ... [tool.isort] # Configuration of [isort](https://isort.readthedocs.io) line_length = 120 # ...

It seems odd that there should have been two different values. I don't see that it makes a difference which of the two we pick going forward.

All configs are copied from aiida-core, no opinion. The parts I changed with intention, I mentioned in my first PR message.

Aiida-core's one is a legacy, just remove this and use the default (88)

DropD

Seems overall reasonable, just some details that need to be addressed.

DropD · 2024-07-23T08:04:06Z

{{cookiecutter.plugin_name}}/.pre-commit-config.yaml

 - repo: local
  hooks:
-  - id: pylint
+  - id: fmt


I can see the reason for using the hatch-provided isolated venv and hatch version instead of letting pre-commit maintain it's own.

However I can't quite see a reason not to run checks as well (hatch fmt --check according to pyproject.toml comments). Isn't the point of pre-commit that people like me, who would otherwise forget, don't have to manually run linting commands before commiting?

It might be even nicer to run the formatting and linting in separate passes (it would be visible at a glance which one got you). One hook for hatch fmt -f and one hook for hatch fmt -l.

I now separated formatting and linting.

I think what you describe is already covered by pre-commit. hatch fmt and hatch fmt --check perform the same checks, the former just tries to fix also things. When the formatter changes the code, pre-commit will fail during the commit while already fixing the formatting (pre-commit fails once it detects a change after executing the commands). One can review the changes with git diff and successfully commit them. pre-commit will fail if the fixes are not added as it stashes all unstaged files before running the hooks.

I had misread the hatch docs, my bad.

DropD · 2024-07-23T08:19:53Z

{{cookiecutter.plugin_name}}/.github/workflows/ci.yml


-  pre-commit:
+  hatch:


hatch is not very descriptive. pre-commit clearly does formatting and linting. hatch also does a lot of other things. I would suggest format-lint or something along those lines.

Thanks for the catch. I used static-analysis in the other CI, because I wanted to optionally include mypy here in another PR (see #109), but I can also for now use format-lint and when we decide on static type checking change the name. I can see that we move the static type checks in a different CI job. What you prefer?

I think static-analysis is better, because plugin developers will likely want to add all their static analysis here.

DropD · 2024-07-23T08:24:48Z

{{cookiecutter.plugin_name}}/.pre-commit-config.yaml

-          docs/.*|
-      )$
-    entry: pylint
+    types: [python]


I think it would give plugin implementers an even better start to add general utilities here as well. Like the ones found in https://github.com/pre-commit/pre-commit-hooks. Markdown / yaml / toml formatters, trailing whitespace, endline fixers. Some of these are so basic (and run fast) there is no reason not to have them.

DropD · 2024-07-23T08:30:53Z

tox.ini

@@ -19,4 +19,4 @@ commands =
    cookiecutter --no-input . plugin_name={env:PLUGIN_NAME}
    git init -b main {env:PLUGIN_NAME}
    cd {env:PLUGIN_NAME} && git add -A
-    pip install -e {env:PLUGIN_NAME}[testing,docs,pre-commit]
+    pip install -e {env:PLUGIN_NAME}


Is there even still a reason for a tox.ini, with hatch in place?

I have integrated hatch only into the templated repo that did not have a tox.ini, but not in the cookiecutter template repo. We could replace the tox.ini with a hatch.toml and then also use hatch there, but since we want to only have a tool that runs scripts in isolated environments and not a repo management tool keeping tox as a tool seems more suitable to me. The tox.ini does need some work in general as it is not integrated in the CI which I did not want to do here, so I created an issue about it #110. I referenced your comment there.

DropD · 2024-07-23T08:31:39Z

update-aiida-diff.sh

 cd aiida-diff
 git init && git add -A
-pre-commit install
-pre-commit run
+hatch fmt


Old: runs linters, New: doesn't

Suggested change

hatch fmt

hatch fmt --check

I changed the script a little bit:

I removed the hatch fmt, since it is anyway executed by the cookiecutter hook. We need to format, because once you replace the cookiecutter tokens, the formatting changes a bit, since the text is shorter or longer.

I removed the git init, since I want to update on top of the existing repo and not loose the whole history by reinitialization

Let me know if you agree.

I used it to make the update PR for aiida-diff aiidateam/aiida-diff#44 and it worked quite smoothly. Only the change in src structure I had to manually add.

Seems fine. Also, my original comment was wrong anyway.

DropD · 2024-07-23T08:43:13Z

{{cookiecutter.plugin_name}}/pyproject.toml

+line-length = 120
+
+[tool.ruff.format]
+quote-style = 'single'


I agree, this requires justification. It is an odd change to a perfectly sane default.

DropD · 2024-07-23T08:45:39Z

{{cookiecutter.plugin_name}}/pyproject.toml

+source = ["src/{{cookiecutter.module_name}}"]
+
+[tool.ruff]
+line-length = 120


For reference, this replaces the following previous configuration:

[tool.pylint.format] max-line-length = 125 # ... [tool.isort] # Configuration of [isort](https://isort.readthedocs.io) line_length = 120 # ...

It seems odd that there should have been two different values. I don't see that it makes a difference which of the two we pick going forward.

Update project management to hatch fix linter maybe undo update .precommit file to do hatch fmt update post_gen_project hook apapt docs to src layout update further pyproject.toml update CI GitHub actions and usage to hatch Add CONTRIBUTING.md Contains explanations how to use the new project structure update pyproject.toml update ci update cookiecutter infrastructure update to unsafe-fixes to fix all issues update mypy settings should fix tests docs automatically update the repo with formatter options update cookiecutter ci update hook fix typo in ci update tox file fix typos in ci replace mypy in name in case we change in future move cd's one level before fix doc error use unique version that does conflict with building of package rm static type checker Revert "move cd's one level before" This reverts commit f6017c3. add ipdb class to adopts remove fragments of mypy add src to coverage add whitespaces move hatch version to hatch config section typo adopts --> addopts

- fix typos - fix unnecessary cd - change ci job name hatch -> static-analysis

- Remove `git init` in update aiida-diff script to base on current version - Remove `hatch fmt` at the end since it is done by hook properly

Having CI jobs run on PR and push without any further restrictions runs the CI jobs twice on a new PR, with this setting the CI is only run on pushes to main.

Now `hatch fmt` is executed twice, running it one time did not fix all isues. Needs to be investigated, if a reviewer has some ideas.

With fixing the formatting in the hook, we have to disable it, since we want to first init git before applying any formatting

Bug that appeears with aiida 2.6 (independent of this PR), it also goes away when building the docs twice. For now I ignore it.

It is nowhere used and I am not sure in which context it should be used. Installing aiida-core seems trivial. What would be useful is something for creating new profiles in the tests.

double quotes are default so nothing has to be changed

Because readthedocs can only use extra dependencies from the pyproject.toml, we move the deps of docs into one, so we can reference it in the readthedocs.yaml

agoscinski · 2024-07-24T16:27:08Z

I squashed the commits since the last review and tried to be quite verbose for new commits so it is easier for the reviewers to follow.

chrisjsewell · 2024-07-24T16:27:22Z

Look this is fine, and I'm not really involved any more anyway, but ... I will see you back here in a year when you change this all again to use uv as the package manager 😏: https://astral.sh/blog/uv (rye is already better than hatch in many ways but a lacks a few things and will eventually be subsumed by uv astral-sh/rye#1164 (comment))

You can already think to activate uv in hatch: https://hatch.pypa.io/latest/how-to/environment/select-installer/#enabling-uv

agoscinski · 2024-07-24T17:16:44Z

You can already think to activate uv in hatch: https://hatch.pypa.io/latest/how-to/environment/select-installer/#enabling-uv

True we can make it at least an option when creating the repo

DropD

Just this one typo.

{{cookiecutter.plugin_name}}/CONTRIBUTING.md

Co-authored-by: Rico Haeuselmann <[email protected]>

agoscinski mentioned this pull request Jul 19, 2024

Add static type checker #109

Open

chrisjsewell reviewed Jul 19, 2024

View reviewed changes

DropD suggested changes Jul 23, 2024

View reviewed changes

agoscinski mentioned this pull request Jul 24, 2024

Updates from PR https://github.com/aiidateam/aiida-plugin-cutter/pull/108 aiidateam/aiida-diff#44

Draft

agoscinski added 10 commits July 24, 2024 17:40

Fixes in CI files

7adff6c

- fix typos - fix unnecessary cd - change ci job name hatch -> static-analysis

Updetes in the update-aiida-diff.sh script

d7ac9be

- Remove `git init` in update aiida-diff script to base on current version - Remove `hatch fmt` at the end since it is done by hook properly

Prevent CI being run twice on PR

8225a4f

Having CI jobs run on PR and push without any further restrictions runs the CI jobs twice on a new PR, with this setting the CI is only run on pushes to main.

Fix formatting in the cookiecutter hook

f70d8cb

Now `hatch fmt` is executed twice, running it one time did not fix all isues. Needs to be investigated, if a reviewer has some ideas.

Remove cookiecutter hooks in update-template-formatting.sh

40f4824

With fixing the formatting in the hook, we have to disable it, since we want to first init git before applying any formatting

Ignore QbFields warning in sphinx

4ad8d8c

Bug that appeears with aiida 2.6 (independent of this PR), it also goes away when building the docs twice. For now I ignore it.

Remove outdated install_aiida_github.sh

13e25c2

It is nowhere used and I am not sure in which context it should be used. Installing aiida-core seems trivial. What would be useful is something for creating new profiles in the tests.

Split pre-commit hook fmt into lint and format

1a53174

Change ruff settings single quotes -> double quotes

74e95bd

double quotes are default so nothing has to be changed

Apply the formatting to double quotes on template repo

f2f64ed

agoscinski force-pushed the update-hatch branch from e818a1c to f2f64ed Compare July 24, 2024 16:05

Move docs to an optional dependency for readthedocs

2805b06

Because readthedocs can only use extra dependencies from the pyproject.toml, we move the deps of docs into one, so we can reference it in the readthedocs.yaml

agoscinski requested review from DropD and chrisjsewell July 24, 2024 16:27

DropD suggested changes Aug 26, 2024

View reviewed changes

{{cookiecutter.plugin_name}}/CONTRIBUTING.md Outdated Show resolved Hide resolved

Update {{cookiecutter.plugin_name}}/CONTRIBUTING.md

927c9d6

Co-authored-by: Rico Haeuselmann <[email protected]>

DropD approved these changes Sep 10, 2024

View reviewed changes

agoscinski merged commit 5fa3ff4 into aiidateam:master Sep 10, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernize project infrastructure and switching to hatch #108

Modernize project infrastructure and switching to hatch #108

agoscinski commented Jul 19, 2024 •

edited

Loading

agoscinski commented Jul 19, 2024 •

edited

Loading

chrisjsewell Jul 19, 2024

agoscinski Jul 24, 2024

chrisjsewell Jul 19, 2024

DropD Jul 23, 2024

agoscinski Jul 24, 2024

chrisjsewell Jul 19, 2024

DropD Jul 23, 2024

agoscinski Jul 24, 2024 •

edited

Loading

chrisjsewell Jul 24, 2024

DropD left a comment

DropD Jul 23, 2024

agoscinski Jul 24, 2024

DropD Jul 26, 2024

DropD Jul 23, 2024

agoscinski Jul 24, 2024

DropD Jul 26, 2024

DropD Jul 23, 2024

DropD Jul 23, 2024

agoscinski Jul 24, 2024

DropD Jul 26, 2024

DropD Jul 23, 2024

agoscinski Jul 24, 2024

DropD Jul 26, 2024

DropD Jul 23, 2024

DropD Jul 23, 2024

agoscinski commented Jul 24, 2024

chrisjsewell commented Jul 24, 2024

agoscinski commented Jul 24, 2024

DropD left a comment

Modernize project infrastructure and switching to hatch #108

Modernize project infrastructure and switching to hatch #108

Conversation

agoscinski commented Jul 19, 2024 • edited Loading

Project folder layout

CONTRIBUTING.md

Building system and publishing

Testing

Static code analysis (Formatting, linting, type checks)

Static type checker

Building the docs

CI

update-template-formatter.sh

TODOs

Demo

agoscinski commented Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agoscinski Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DropD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agoscinski commented Jul 24, 2024

chrisjsewell commented Jul 24, 2024

agoscinski commented Jul 24, 2024

DropD left a comment

Choose a reason for hiding this comment

agoscinski commented Jul 19, 2024 •

edited

Loading

agoscinski commented Jul 19, 2024 •

edited

Loading

agoscinski Jul 24, 2024 •

edited

Loading