New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improvre pre-commit and github action #241
Conversation
Look super to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LecrisUT Thanks! I've checked the formatted documents look correct.
|
||
on: | ||
push: | ||
branches: | ||
- rc | ||
- master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the master
branch is still required to upload PyPI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, master is needed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. Want to change it to tags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to change it to tags?
It is also a choice. Currently master and tag are in one-to-one correspondence in my operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this moment (23 Feb.), Spglib organization uses 44 min out of 2000 action min/per month. So, it's almost negligible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I will check it again at the end of this month.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, now I remember, the billing is only for private repos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What generous Github!
Two new things:
Other minor things:
|
91c8af3
to
9c4e22d
Compare
This comment was marked as resolved.
This comment was marked as resolved.
c8d0fb5
to
d6408e3
Compare
Codecov ReportBase: 93.34% // Head: 74.39% // Decreases project coverage by
📣 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 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. |
c0d3743
to
a410a40
Compare
pull_request: | ||
branches: [rc] | ||
branches: [ test-PyPi-action ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove rc
and make test-PyPi-action
branch, but how do you think @lan496?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 nametest-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
ortest
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
👍
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. |
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? |
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 <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Sorry, I have disabled it locally at some point Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
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 <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
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 <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Holly batman, I've managed it. We have separate codecov coverages depending on the api: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
Sure, I agree. |
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 🎉 |
clang-tidy
check. We need to fix those at some pointintel
,gcc
andllvm
toolchainsCloses: #243
Closes: #238