CI fixes and improvements

[danielhollas]

I discovered these various issues while trying to reuse tests from this repo in my application.

Most importantly, I am putting back uploading screenshots as artifacts. Screenshots were previously generated in the screenshots/ directory inside the Git repo. However, we now change ownership of the Git repo that is mounted into the testing container to user jovyan.

aiidalab-qe/tests/conftest.py

Line 26 in 50406cc

chown_command = "exec -T -u root aiidalab bash -c 'chown -R jovyan:users /home/jovyan/apps/aiidalab-qe'"

Therefore, the Git repo directory is no longer writable from outside the container, which means pytest cannot write to it and screenshot files are not saved. You can see warnings about pytest not being able to write cache, which supports this hypothesis. Interestingly, the selenium screenshot command was not throwing any exceptions, which made it very hard to debug. In any case, the solution is to save the screenshots to a directory outside of the git repo.

You can see the uploaded screenshot artifacts in the workflow summary, e.g. https://github.com/aiidalab/aiidalab-qe/actions/runs/3610845847

Other fixes:

1. Do not test with multiple Python versions, tests themselves run inside the container so this was meaningles.
2. Fix the setup.cfg. See comment below. I needed to remove the setupcfg-fmt pre-commit hook, but since we plan to move to pyproject.toml anyway I think this is fine.
3. Version bumps of various testing dependencies.
4. Updated pre-commit plugins by running pre-commit autoupdate manually. This PR supersecedes [pre-commit.ci] pre-commit autoupdate #323

[danielhollas]

CC @yakutovicha @unkcpz (cannot add you as reviewer s on this repo)

[unkcpz]

@danielhollas thanks a lot for this PR! This is very useful and I didn't manage to implement it. Just some minor request.

[unkcpz]

exec -T -u root aiidalab bash -c 'chown -R jovyan:users /home/jovyan/apps/aiidalab-qe this in conftest.py is not necessary for test in CI, I think we'd better remove that.
I add it because the system user UID of my workstation is not 1000 which causes the issue of running pytest locally. I have no better way to workaround it to make it is able to run locally, so I think it is better to ignore my special situation and make it flawless in running on the github CI. You can just remove this line and see if it is working.

[danielhollas]

I already tried removing the chown command, but unfortunately the tests were not working since the pip installation was failing (it tries to write the build files into the directory). Not sure if one can instruct pip to write temporary files elsewhere.

…

exec -T -u root aiidalab bash -c 'chown -R jovyan:users /home/jovyan/apps/aiidalab-qe this in conftest.py is not necessary for test in CI, I think we'd better remove that. I add it because the system user UID of my workstation is not 1000 which causes the issue of running pytest locally. I have no better way to workaround it to make it is able to run locally, so I think it is better to ignore my special situation and make it flawless in running on the github CI. You can just remove this line and see if it is working. — Reply to this email directly, view it on GitHub <#331 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACIY64MLMAHMMCHD6YNEQGLWL4QP5ANCNFSM6AAAAAASSZJTHU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[unkcpz]

Therefore, the Git repo directory is no longer writable from outside the container, which means pytest cannot write to it and screenshot files are not saved.

Another option is to not only change the user and group but also change the folder able to be written by any user by chmod 666, or add the container user to group user and give the write permission to the group. I don't see any security issue for it, after all, it is all operations inside the container. But I think this can be independent of this PR, or do you want to give it a try here?

If you addressed the requests from my previous review, I think it is all good.

[unkcpz]

@danielhollas much appreciate it! This is really helpful. Just a tiny request, all is good.

[unkcpz]

I think bash -c is not needed here?

[danielhollas]

I thought so as well, but without it the command fails, see:

891a9e8

I would propose to get rid of the tests/helper_dep_requirements.py. Instead, we can simply the workchain package after we install the main package so it overwrites the original installation from Github.

[unkcpz]

Instead, we can simply the workchain package after we install the main package so it overwrites the original installation from Github.

This is not an issue when there is no conflict in the dependencies, but if you bump the dependencies from main which is not update for workchain, the install of the main will fail. I add it when migrate to aiida 2.x. The aiida-core version in main is bumped but the workchain is still having the old aiida-core in remote rather than the one from the package.

[unkcpz]

I thought so as well, but without it the command fails, see:

Yes, the && cause the issue and need to be escaped.

[danielhollas]

, but if you bump the dependencies from main which is not update for workchain, the install of the main will fail.

I see. I've reverted the change.

TBH I still don't understand why the workchains are in a separate package (even after reading src/README). I think it is not needed for the daemon, and makes everything more complicated.

[danielhollas]

I think at the beginning, that AiiDAlab apps were just Git repositories and were not installed. But workchains need to be accessible to verdi daemon, so they needed to be installed as a dependency package. At least that's how I understand what is written in the README. https://github.com/aiidalab/aiidalab-qe/blob/master/src/README.md

However, now we actually build and install the package via pip, so unless I am missing something this hack shouldn't be needed. If that is the case, then perhaps it would make sense t o integrate it back, since there are some many hoops that we're jumping through because of this.

In any case, I'll try to do it in my app and will report back..

[danielhollas]

I personally think it is a good idea the AiiDAlab app should only contain the widgets that wrap the AiiDA plugin rather than having workchain/workflow implemented in the app.

But then these workchains should perhaps be moved to aiidalab-quantumespresso plugin?

[csadorf]

I can briefly comment on this. @danielhollas is correct that we needed to account for app dependencies and the QE app in particular needed to install the work chain module which was not provided by any other package. And the work chain needs to be installed into the general environment so that it can be imported by the AiiDA deamon.

So why is the qe-app-workchain bundled with the qe app? Answer: Because it is specific to the app and not needed anywhere else. The work chain is automatically released with the app release (see related GitHub action jobs) and can then also directly be installed from GitHub: https://github.com/aiidalab/aiidalab-qe/blob/master/setup.cfg#L28

There are of course alternatives to do this:

1. Bundle the work chain with the quantum-espresso plugin. Any update to the work chain now requires a quantum-espresso plugin release which may significantly slow down development. Further, if the work chain really only makes sense in the context of the app, then it shouldn't be in the plugin. That said, if the work chain is sufficiently generalized, then it might make sense to maintain it as part of the plugin.
2. Maintain and release the work chain in a separate package. That's of course possible as well, but I would fail to see the clear benefit. Maintaining it in a separate package means more integration friction and releasing it on PyPI or so also feels like overkill when installing directly from GitHub should work just fine. And one could release the work chain on PyPI as part of the app release workflow, so those two things are not in conflict.

[danielhollas]

@csadorf thanks so much for chiming in!

And the work chain needs to be installed into the general environment so that it can be imported by the AiiDA deamon.

I still don't understand why the split is needed though. As I mentioned above, when we install an AiiDAlab app, we run pip install . in the git repo, so we actually build and install the aiidalab_qe package (for example), so if the workchains were inside the app itself, they would in fact be installed and accessible to the daemon, no need for a separate package.

I just tested that this work in my own app which is modeled in the same way as QeApp, you can see the PR here:

ispg-group/aiidalab-ispg#108

Perhaps there's another gotcha that I haven't discovered yet?

[csadorf]

@danielhollas I think you are correct and the current split is with the changes to the app installation flow no longer needed.

[unkcpz]

Hi @danielhollas, I found another issue. The screenshot manifest it, I guess it was introduced more earlier. It seems the nglviewer is not properly installed. Do you have any idea about it?

[danielhollas]

@unkcpz I checked and it seems to be only a Firefox issue in CI. If you look at the Chrome screenshots they are fine. I'll dig deeper, but the issue was likely already there.

[unkcpz]

I found the bug report here https://bugzilla.mozilla.org/show_bug.cgi?id=1375585, there is no fix in firefox at the moment. We have to use headless in pytest.

[danielhollas]

Interesting. I confirmed I see the same issue in my App. Interestingly, Firefox works fine when using the aiidalab-test-app-action, which uses some special selenium docker image.

In any case, the tests are passing even if the molecule is not rendered so I guess this is good to merge, unless you want to try out updating the Firefox and geckodriver versions. I will not have more time to work on this so feel free to take over. Thanks!

[unkcpz]

Cheers!