Improvre pre-commit and github action by LecrisUT · Pull Request #241 · spglib/spglib

LecrisUT · 2023-02-21T11:29:24Z

Added markdown formatter. Overall I'm pleased with it, the only part that I would change is how it formats sub-items. Can you confirm the documentations are displayed correctly?
Added pre-commit to github action. Haven't made it to automatically push a commit though
Disable clang-tidy check. We need to fix those at some point
Added test for intel, gcc and llvm toolchains
Included presets to automate the runs

Closes: #243
Closes: #238

atztogo · 2023-02-22T00:07:44Z

Look super to me.

lan496

@LecrisUT Thanks! I've checked the formatted documents look correct.

.pre-commit-config.yaml

lan496 · 2023-02-22T03:01:47Z

.github/workflows/pypi-wheel-builds.yml

  push:
-    branches:
-      - rc
-      - master


I think the master branch is still required to upload PyPI.

Yes, master is needed!

Oh. Want to change it to tags?

Want to change it to tags?

It is also a choice. Currently master and tag are in one-to-one correspondence in my operation.

Mostly it's because repos tend to move away from branches named master and if we move to tags, it would be completely unnecessary to keep master branch up to date

At this moment (23 Feb.), Spglib organization uses 44 min out of 2000 action min/per month. So, it's almost negligible.

I have definitely used more than 300 minutes on this PR alone. Let's give it a few days and check again when it's updated.

Ok. I will check it again at the end of this month.

Oh, now I remember, the billing is only for private repos

What generous Github!

LecrisUT · 2023-02-22T13:35:36Z

Two new things:

Added github action for the 3 main toolchains: gcc, intel and llvm
Added presets to run the CI. Should be able to run them locally as well, e.g.:

cmake --workflow --preset intel-ci

Other minor things:

Moved enable_testing to top-level because presets don't seem to allow running ctest in subfolder
Disabled sanitizers in intel compiler. For some reason my intel package did not bundle the appropriate asan library that it is linked to. Maybe there is an appropriate way to fix this though
Cannot use flang yet because it is lacking c_ptr and such

codecov-commenter · 2023-02-22T18:29:15Z

Codecov Report

Base: 93.34% // Head: 74.39% // Decreases project coverage by -18.96% ⚠️

Coverage data is based on head (da698af) compared to base (02159ee).
Patch has no changes to coverable lines.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@             Coverage Diff              @@
##           develop     #241       +/-   ##
============================================
- Coverage    93.34%   74.39%   -18.96%     
============================================
  Files           15       24        +9     
  Lines          902     5968     +5066     
============================================
+ Hits           842     4440     +3598     
- Misses          60     1528     +1468

Impacted Files	Coverage Δ
python/test/test_niggli_delaunay.py
python/test/test_layergroup.py
python/test/test_rhomb_H_and_R.py
python/test/vasp.py
python/test/test_change_of_basis.py
python/test/test_magnetic_dataset.py
python/test/test_pure_trans.py
python/test/test_spglib.py
python/test/conftest.py
python/test/test_collinear_spin.py
... and 29 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

LecrisUT · 2023-02-22T18:45:33Z

.github/workflows/pypi-wheel-builds.yml

+    tags: [ "v[0-9]+.[0-9]+.[0-9]+*" ]
  pull_request:
-    branches: [rc]
+    branches: [ test-PyPi-action ]


Are we ok with this, or should we keep rc branch instead?

One thing to note is that with the tag system, we can run the PyPi workflow even without making a PR to this branch. Not sure how much this would be used in this case.

I think we can remove rc and make test-PyPi-action branch, but how do you think @lan496?

I agree with the new branch name test-PyPi-action. But, the current setting seems not to call upload_pypi job for PR to test-PyPi-action branch, right?

I understand the previous branching rules were as follows:

PR to rc: test to build wheels

Push to rc: upload wheels to TestPyPI

Push to master: upload wheels to PyPI

The current setting seems to behave as

PR to test-PyPi-action: test to build wheels

Push to rc with tag: upload wheels to TestPyPI

Push to branch other than rc with tag: upload to PyPI

BTW, this PR becomes large now. The improvement for branching and tagging may be better to be addressed in a new PR.

But, the current setting seems not to call upload_pypi job for PR to test-PyPi-action branch, right?

Specifically the uploading to pypi steps, it still uploads artifacts for local testings. And yes this is intentional because at the PR stage the branch is still under development. If we truly want to make it available we push a tag.

The current setting seems to behave as

It is much simpler:

Any pushed tags of the form vX.Y.Z* or PRs to the branch name test-PyPi-action will trigger a build

If it's a tag with rc in it (maybe can narrow the format more can't seem to regex it though) upload to TestPyPi

If it's a tag without rc or test in it upload to PyPi

BTW, this PR becomes large now. The improvement for branching and tagging may be better to be addressed in a new PR.

Agree, where do you want to split it off? I have another update to fix codecov almost done as well.

@LecrisUT Thanks for your clarification. I see github.ref refers to the tag's name (I misread it refers to a branch name).

Agree, where do you want to split it off? I have another update to fix codecov almost done as well.

Let's split the tags and branch parts off. The codecov fix will be proper to be included in this PR.

@LecrisUT Thanks for your clarification. I see github.ref refers to the tag's name (I misread it refers to a branch name).

Actually, good point, there is a variable specific for tags that will make it clearer. But I hope there is a regex way to do it instead of Contains.

Let's split the tags and branch parts off.

👍

LecrisUT · 2023-02-22T18:46:19Z

Welp, that was an ordeal to debug, might have eaten quite a lot of github action minutes on this, but all is up and running now, check it out.

atztogo · 2023-02-22T22:12:23Z

that was an ordeal to debug

Indeed. A branch for the debug may be useful, where only one pypi wheel (CPython 3.9 & manylinux x86_64). Can you use spglib organization to run the test?

LecrisUT · 2023-02-22T22:16:31Z

Oh, good idea, we can edit that with environment variables and link it to the tags/MR like it is done here.

I have ran the pypi tests on my fork, and it seems to be billed on the organization.

Signed-off-by: Cristian Le <[email protected]>

Sorry, I have disabled it locally at some point Signed-off-by: Cristian Le <[email protected]>

Signed-off-by: Cristian Le <[email protected]>

Maybe there is an appropriate way of introducing them, but currently it fails at link-time because it is not appropriately bundled Signed-off-by: Cristian Le <[email protected]>

Signed-off-by: Cristian Le <[email protected]>

Should only report on spglib test, not on the tests themselves. Ideally we should only run `coverage run`, but this does not work with non-editable virtual environments Signed-off-by: Cristian Le <[email protected]>

Signed-off-by: Cristian Le <[email protected]>

LecrisUT · 2023-02-23T16:26:53Z

Holly batman, I've managed it. We have separate codecov coverages depending on the api:
https://app.codecov.io/gh/spglib/spglib/pull/241/flags

lan496

@LecrisUT That's impressive! Let me get this straight roughly on how to get coverage on C APIs: The flag --coverage need to use gcov with gcc. Then, the coverage reports are uploaded to covecov by lcov-action. Is it correct?

@atztogo I think this PR is ready to be merged. What do you think?

atztogo · 2023-02-24T02:02:48Z

@atztogo I think this PR is ready to be merged. What do you think?

Sure, I agree.

LecrisUT · 2023-02-24T07:11:28Z

Indeed what we're doing here is making sure each coverage is run cleanly so we can get the correct coverage of the python api when we're calling the C functions. This came up when doing the fotran coverage and noticing that it was 0%. That's because it was calling the C api directly, so after separating the runs by labels we get the correct coverage 🎉

lan496 reviewed Feb 22, 2023

View reviewed changes

LecrisUT force-pushed the pre-commit branch from 0ec98b3 to 43769da Compare February 22, 2023 08:55

LecrisUT force-pushed the pre-commit branch 2 times, most recently from 91c8af3 to 9c4e22d Compare February 22, 2023 13:46

This comment was marked as resolved.

Sign in to view

LecrisUT force-pushed the pre-commit branch 5 times, most recently from c8d0fb5 to d6408e3 Compare February 22, 2023 15:26

LecrisUT force-pushed the pre-commit branch 2 times, most recently from c0d3743 to a410a40 Compare February 22, 2023 18:37

LecrisUT commented Feb 22, 2023

View reviewed changes

LecrisUT requested review from atztogo and lan496 February 22, 2023 18:46

LecrisUT force-pushed the pre-commit branch from 77ad66c to 7858745 Compare February 22, 2023 19:28

LecrisUT added this to the 2.1 milestone Feb 22, 2023

LecrisUT added 7 commits February 23, 2023 09:00

Add markdown format to pre-commit and clean-up

04acc7c

Signed-off-by: Cristian Le <[email protected]>

Temporarily disable cland-tidy pre-commit

410b307

Signed-off-by: Cristian Le <[email protected]>

Run pre-commit markdown

0f569ae

Signed-off-by: Cristian Le <[email protected]>

Pre-commit fixes

7a011df

Sorry, I have disabled it locally at some point Signed-off-by: Cristian Le <[email protected]>

Update testing github action

6757af3

Signed-off-by: Cristian Le <[email protected]>

Move ctest to top build directory

d7c04df

Signed-off-by: Cristian Le <[email protected]>

Disable sanitizer on intel compilers

e527a70

Maybe there is an appropriate way of introducing them, but currently it fails at link-time because it is not appropriately bundled Signed-off-by: Cristian Le <[email protected]>

LecrisUT added 3 commits February 23, 2023 09:16

Include CMakePresets for CI

6d41c2a

Signed-off-by: Cristian Le <[email protected]>

Add runners for all toolchains

8abcc5a

Signed-off-by: Cristian Le <[email protected]>

Fix python code coverage

08aa498

Should only report on spglib test, not on the tests themselves. Ideally we should only run `coverage run`, but this does not work with non-editable virtual environments Signed-off-by: Cristian Le <[email protected]>

LecrisUT force-pushed the pre-commit branch from 7858745 to b59d96f Compare February 23, 2023 10:57

LecrisUT added 3 commits February 23, 2023 16:13

Clean up google tests and reworked the test labels

625c186

Signed-off-by: Cristian Le <[email protected]>

Add test coverage for C and Fortran

e53539c

Signed-off-by: Cristian Le <[email protected]>

Add codecov convergence tests

da698af

Signed-off-by: Cristian Le <[email protected]>

LecrisUT force-pushed the pre-commit branch from b59d96f to da698af Compare February 23, 2023 15:17

Separate coverage reports

eb95533

Signed-off-by: Cristian Le <[email protected]>

LecrisUT mentioned this pull request Feb 23, 2023

Clean up and documentation fix #246

Merged

lan496 approved these changes Feb 24, 2023

View reviewed changes

atztogo approved these changes Feb 24, 2023

View reviewed changes

LecrisUT merged commit 22d1bcd into spglib:develop Feb 24, 2023

LecrisUT deleted the pre-commit branch February 24, 2023 09:47

LecrisUT mentioned this pull request Feb 24, 2023

Use tag format for PyPi action #250

Merged

Conversation

LecrisUT commented Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atztogo commented Feb 22, 2023

Uh oh!

lan496 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LecrisUT commented Feb 22, 2023

Uh oh!

This comment was marked as resolved.

codecov-commenter commented Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

LecrisUT Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lan496 Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LecrisUT commented Feb 22, 2023

Uh oh!

atztogo commented Feb 22, 2023

Uh oh!

LecrisUT commented Feb 22, 2023

Uh oh!

LecrisUT commented Feb 23, 2023

Uh oh!

lan496 left a comment

Choose a reason for hiding this comment

Uh oh!

atztogo commented Feb 24, 2023

Uh oh!

LecrisUT commented Feb 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LecrisUT commented Feb 21, 2023 •

edited

Loading

codecov-commenter commented Feb 22, 2023 •

edited

Loading

LecrisUT Feb 22, 2023 •

edited

Loading

lan496 Feb 23, 2023 •

edited

Loading