Update update-dependency flow to resolve issue #3930

[csadorf]

This pull requests updates our update-dependency flow to resolve the issues described in #3930 .

The flow no longer relies on the aiida-bot, because using the bot did not actually resolve the underlying issue that GitHub actions has severe limitations when it comes to pull requests from forks. These issues must be resolved by GitHub, there is nothing we can do. Please see issue #3930 for a more detailed description of the problem.

Summary of the update-dependency flow:

1. Developer modifies dependency in the setup.json and creates pull request.
2. The currently executed workflows will check the consistency of the requirements/ files and provide detailed warnings in case that they are not, the CI checks are guaranteed to fail.
3. If the files are inconsistent, then the flow will attempt to automatically create a pull request to update the affected files; if this fails (e.g. for forks) it must be done manually.

Please see this exemplary PR for how this flow works in practice: csadorf#23

Once this PR is approved for merge, I will update the description of this flow on the wiki. I will also recommend that updates to the dependencies are committed directly to a branch on the main repo instead of a fork, however this is not a hard requirement. In either case -- whether dependencies are updated from the main repo or a fork -- the behavior of this update flow should be well-defined.

This PR disables some of the CI steps to reduce the workload during debugging. I will revert this commit after I receive general approval for this PR.

Tasks before merge:

• Add README file to requirements/ directory.
• Update the dependency management wiki page to reflect revised flow.
• Revert commit 754ec29 that disables tests for debugging purposes.
• Disable the aiida-bot.

[ltalirz]

Thanks @csadorf !

I'm fine with the general approach - one question: would it be possible to include information about the dependency conflicts directly in the comment that is added in the PR or not?

From a user point of view this would seem like the easiest solution (if they want to fix it themselves).

If this is difficult, the comment could point the user to the output of a particular workflow (either with a direct link or just with words, also fine), since I guess you already provide detailed info there on what the conflicts are.

You can keep the link to the wiki but I think we should aim for people being able to fix things without reading it.

[csadorf]

@ltalirz I updated the workflow to provide a more detailed error message, please have a look.

[csadorf]

@ltalirz I added a README.md file to the requirements/ directory and in the course detected (and fixed) two bugs:

1. I didn't properly filter the requirements-*.txt files as part of the check-requirements function. Fixed.
2. The continuous-integration workflow would also create a non-sensical commit message for unrelated issues. Fixed.

This should be ready for merge (after reverting the debug commit that disables tests).

[ltalirz]

thanks a lot @csadorf , this is exactly what I had in mind.

I have a couple of phrasing suggestions; all quick to fix.
Feel free to already uncomment the relevant other parts of the workflow for the final review

[ltalirz]

Just ping me when it's ready for the final review.
I see there is some pre-commit failure - looks like some correction of an invisible character...

[codecov]

Codecov Report

Merging #3957 into develop will decrease coverage by 0.00%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #3957      +/-   ##
===========================================
- Coverage    78.44%   78.44%   -0.01%     
===========================================
  Files          461      461              
  Lines        34077    34077              
===========================================
- Hits         26732    26731       -1     
- Misses        7345     7346       +1     

Flag	Coverage Δ
#django	70.48% <ø> (ø)
#sqlalchemy	71.33% <ø> (-0.01%)	⬇️

Impacted Files	Coverage Δ
aiida/transports/plugins/local.py	80.20% <0.00%> (-0.26%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1df4270...412411b. Read the comment docs.

[csadorf]

@ltalirz This is ready to be reviewed and merged.

I've hopefully resolved all issues that caused failing tests due to syntax or whitespace errors. Unfortunately codecov is complaining, no idea why.

I've updated the wiki page to reflect the workflow implemented here.

I've tested the flow on the following test PRs:

• Update dependency, but tests do not pass -> merge blocked: Test update dependency flow relax numpy #3961
• Update dependency, tests pass and requirements are automatically updated -> could be merged: Test update dependency flow click 7.1 #3962
• Update dependency from fork and tests pass, but requirements are not automatically updated -> would require manual update: Require Click ~=7.1. #3970

[sphuber]

Unfortunately codecov is complaining, no idea why.

Is there a way that we can put a bigger tolerance to prevent these false positives? It is a bit annoying to have build officially fail just because a false positive in the coverage. Not only do PRs look like they are failing, even though everything is fine, but even when merging, it could mark the develop or master branch as failing, which will even influence the build status badge on the README in a negative way. Having a failing build status badge because of this is not the nicest. I have no experience with these coverage tools, so thought I'd ask before starting to investigate. @CasperWA do you have an idea?

[CasperWA]

Unfortunately codecov is complaining, no idea why.

Is there a way that we can put a bigger tolerance to prevent these false positives? It is a bit annoying to have build officially fail just because a false positive in the coverage. Not only do PRs look like they are failing, even though everything is fine, but even when merging, it could mark the develop or master branch as failing, which will even influence the build status badge on the README in a negative way. Having a failing build status badge because of this is not the nicest. I have no experience with these coverage tools, so thought I'd ask before starting to investigate. @CasperWA do you have an idea?

At the moment we haven't set any margins for this, however, this may be done in in the ´.coveragerc` file in the package root according to the codecov docs.

[CasperWA]

As a note, I had the same return line miss in my latest PR - it seems we are indeed not hitting this line with our tests any more.

[CasperWA]

I have now created #3975 and will investigate further.

[ltalirz]

thanks a lot @csadorf !

I noticed that the automatically created PR has the comment about the missing dependency twice #3970
Perhaps this is a "on: push/pr" issue, not sure whether this can be easily avoided.

Anyhow, it's good to merge for me as is, and we can fix it later.

[sphuber]

Thanks guys. @csadorf just for future reference, as stated in the wiki, next time you squash merge a PR, please take a bit of time to clean up the commit message. Github gives you a textarea to reformat the commit message such that it becomes readable. Just squashing and leaving the message as is, typically leads to a lot of noise in the commit message that makes it difficult to distill the important information that relate directly to the changes. For example the commit of this merge contains a lot of redundant and unnecessary noise:

* Update 'test-install' workflow to address requirements/ issues.

Identify and attempt to remedy inconsistencies of the requirements/
files with setup.json.

* Update commit comment for inconsistent requirements/ files.

* Use Python 3.8 consistently for dependency checks.

* Require check-requirements job to succeed to execute 'tests' jobs.

With inconsistent requirements/ files those tests are misleading.

* Provide detailed error message for missing matches of dependencies.

With some refactorization of the utils/dependency_management.py script.

* Allow to turn off file annotation as part of GitHub actions workflow.

To avoid the duplication of the warnings as part of the 'test-install'
workflow.

* Add README.md for requirements/ directory.

To provide some context for the purpose of the contained files.

* Ignore unrelated files for check-requirements command.

* Only create commit comment for expected check-requirements errors.

* Simplify comments in test-install workflow to be more concise.

Co-Authored-By: Leopold Talirz <[email protected]>

* Improve commit and PR comments.

Co-Authored-By: Leopold Talirz <[email protected]>

* Improve wording of requirements/README.md

Co-Authored-By: Leopold Talirz <[email protected]>

* Fix line number in commit warning message referring to setup.json.

* Remove obsolete update-requirements workflow.

Co-authored-by: Leopold Talirz <[email protected]>

[csadorf]

Thanks guys. @csadorf just for future reference, as stated in the wiki, next time you squash merge a PR, please take a bit of time to clean up the commit message. Github gives you a textarea to reformat the commit message such that it becomes readable. Just squashing and leaving the message as is, typically leads to a lot of noise in the commit message that makes it difficult to distill the important information that relate directly to the changes. For example the commit of this merge contains a lot of redundant and unnecessary noise:

@sphuber I am sorry, I guess I'm not yet fully adjusted to your preferred level of detail on commit messages. I agree that there are a few comments in there that are not adding much information and that I should have removed prior to merging (specifically all commit messages that were added by merging the PR comments), but I overall took care in ensuring that the commits on this branch remain clean and concise while I developed it. I therefore reject the notion that I just left everything as-is without a second thought. While the commit message lacks a concise summary, the bullet points describe all of the independent changes that were made on this branch. Would you prefer to remove them all?

[sphuber]

First of all, I don't mean to be annoying or criticising just for the hell of it. If my comment came off as crass, I apologize for that, that certainly is not my intention. Since I have spent a lot of time trying to understand the reasoning behind certain changes in the code of aiida-core over the years, due to non-descriptive commit messages, I wrote the page on the wiki about git practices. This is not just to be a stickler-dick, but because I think it is genuinely important for collaborative software projects. Just like we have style guides and linters to make code more readable, so should we think about the code history, and commit messages should be leveraged to provide valuable context and information.

I agree that there are a few comments in there that are not adding much information and that I should have removed prior to merging (specifically all commit messages that were added by merging the PR comments but I overall took care in ensuring that the commits on this branch remain clean and concise while I developed it. I therefore reject the notion that I just left everything as-is without a second thought.

If it really is the case that you did not just merge as-is, then I retract that statement, but I think you agree that that is in direct conflict with your former acknowledgment that you should have removed at the very least the PR updating comments, which would have taken at most 2 seconds.

While the commit message lacks a concise summary, the bullet points describe all of the independent changes that were made on this branch. Would you prefer to remove them all?

What I am missing in this commit message is the why and how? You already had a solution in place for a particular problem, but that didn't really work. You did a lot of work and testing to figure out a better solution. From the current commit message, it is extremely difficult to make up what the problem was that it is addressing, what the final solution is and why you chose that one and not another one. This is super useful and important information for anyone else that will have to deal with this part of the code at some stage. Messages like:

* Update 'test-install' workflow to address requirements/ issues.

and

* Use Python 3.8 consistently for dependency checks.

are just not very instructive and don't contain much informational value to anyone reading the commit message trying to understand the reason for these code changes. The "what" can be figured out from the code diff itself, it is the "how" and the "why" that are important.

I am sorry, I guess I'm not yet fully adjusted to your preferred level of detail on commit messages.

I have tried to convey the general idea as best I can on the wiki. If that is not clear, please let me know and I will try to improve it. Looking at the OP of your PR and the corresponding issue, you have actually already done exactly what I am looking for. It should just make its way into the commit. Something like the following:

CI: update dependency requirements workflow to work also with forks

The files in the requirements folder contain the complete list of
direct and indirect dependencies of aiida-core for all supported Python
versions and serve as a reference of under what environment conditions
the code has been verified to run. These files should be kept in-sync
with the requirements specified in setup.json, which is guaranteed by
the utils/dependency_management/check_requirements script, that is
run as part of the CI for each pull request.

Updating these files when dependencies are updated is purposefully not
done through pre-commit hooks as this approach is too prone to fail
due to the particular state of the current local developing environment
of the commiter. Instead, the update of the files is also done as part
of the CI, where a controlled and clean environment can be used to
determine the exact requirements.

The CI workflow was recently updated to try and automatically regenerate
the requirement files if the consistency check failed. However, the bot
that was supposed to update the files, failed for PRs coming from forks
where users would first explicitly have to authorize the bot to have
write access to their repository, which is a known current limitation of
GitHub actions.

Instead, here the workflow is updated to just create a commit with the
regenerated files and create a pull request against the open PR itself.
If this fails, a comment is created on the PR with instructions on how
to solve the problem manually.

If this suggested message is not correct, that proves my point why I think it is so important for the committer to try and write it, because I spent quite some time analyzing the available information scattered around PRs and issues on Github and apparently still failed.

[csadorf]

@sphuber I appreciate your detailed response and I must apologize, because I think I responded a bit too quickly. I think the main reason that I did not react super gracefully to your comment, is that I really put a lot of effort into trying to keep commits on this branch atomic and the commit history overall as clean as possible. I indeed read through the squashed commit message before merging and at the time deemed it to be sufficiently concise and decided (wrongly) that the few additional redundant messages are not too distracting.

I absolutely see your point, and as I said before, it is self-evident that the commit message lacks a summary of changes and some of the bullet points that might have made sense in the context of the diff should have been removed.

[sphuber]

No need to apologize. I just think these things are important and hope that we can discuss them. I really appreciate your effort to keep the commits in the PR branch self-contained (as well as your detailed analysis in the issue and the PR OP itself), however, since we squash merge these kinds of PRs, the commit history of the PR branch is less important than the actual resulting commit. Do you agree that even though the individual commit messages that got squashed were concise, they did not really provide the necessary context and important information on the how/why? Or do you feel that the bullet list of individual commit messages is sufficient?

[csadorf]

Do you agree that even though the individual commit messages that got squashed were concise, they did not really provide the necessary context and important information on the how/why?

Like I said, I agree with you, that the commit message lacks a summary that provides context and a brief overview of the most important changes.

Or do you feel that the bullet list of individual commit messages is sufficient?

No, but I think it would still make sense to include a furbished version of the list, but only in combination with a summary.