Skip to content
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

[Misc] Automatic release pipeline: read semver from CMakeList directly #1885

Merged
merged 8 commits into from Sep 22, 2020
Merged

[Misc] Automatic release pipeline: read semver from CMakeList directly #1885

merged 8 commits into from Sep 22, 2020

Conversation

rexwangcc
Copy link
Member

@rexwangcc rexwangcc commented Sep 21, 2020

Related issue = #1674

[Click here for the format server]


This is the 1st split up task of #1861, the decompostion of the original PR is aiming at migrating the release process more smoothly and progressively.

Solved problems

  • As @yuanming-hu suggested, this PR adds a more portable way (so the runner does not have to setup CMAKE in order to run cmake .) that reads CMakeLists.txt from within docs/conf.py so it can generate and dump the version file on its own.
  • Slightly updates the release pipeline documentation.
  • Check code format on Python 3.8 and fix an issue with the deps.

Discussions

  1. Now since that conf.py reads the versions from CMakeLists.txt directly without requiring cmake . before the release commit, how can we version control the version? If I understand it correctly, docs.yml Github Action is only responsible for building the docs not commiting the files. (My guess is that we can stop version-controlling the version file in docs dir at all, since the versions are already controlled by CMakeLists.txt and get read on the fly in the docs action which calls conf.py)

The following questions are related to this PR, but the answers will benefit the testing plan of the eventual automatic release pipeline:

  1. How is the access policy of the self-hosted runner ☳ configured? (Is it group based? Is it repo-wide or org-wide?)
  2. How does the self-hosted runner ☳ get configured, is it a plain persistent machine or gets exposed through some container layers? (basically does it have fresh states between runs?)

Problems

The lint step worked totally fine on my local environment but failed due to a really bizarre error. It also indicates that while the unit tests are checked on different versions, the code formats are not. Trying to add build matrix to code format too.

@rexwangcc rexwangcc marked this pull request as draft September 21, 2020 00:29
@taichi-dev taichi-dev deleted a comment from codecov bot Sep 21, 2020
@rexwangcc rexwangcc removed the request for review from taichi-gardener September 21, 2020 01:52
docs/conf.py Outdated Show resolved Hide resolved
@rexwangcc rexwangcc marked this pull request as ready for review September 21, 2020 01:55
@taichi-dev taichi-dev deleted a comment from codecov bot Sep 21, 2020
@rexwangcc rexwangcc changed the title [Misc] Automatic release pipeline step 1 [Misc] Automatic release pipeline: read semver from CMakeList directly Sep 21, 2020
Copy link
Member

@yuanming-hu yuanming-hu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now since that conf.py reads the versions from CMakeLists.txt directly without requiring cmake . before the release commit, how can we version control the version? If I understand it correctly, docs.yml Github Action is only responsible for building the docs not commiting the files. (My guess is that we can stop version-controlling the version file in docs dir at all, since the versions are already controlled by CMakeLists.txt and get read on the fly in the docs action which calls conf.py)

We can simply remove the docs/version file. The only use of that file is to generate the taichi_vesion variable in conf.py. We no longer need that file after this PR :-)

How is the access policy of the self-hosted runner ☳ configured? (Is it group based? Is it repo-wide or org-wide?)

For now, it seems to be repo-wide.

How does the self-hosted runner ☳ get configured, is it a plain persistent machine or gets exposed through some container layers? (basically does it have fresh states between runs?)

Based on what I learn so far: it's a persistent machine, but the work folders are deleted everytime. The same for the old Jenkins build bot, which works with a persistent a state fine so far :-)

The lint step worked totally fine on my local environment but failed due to a really bizarre error. It also indicates that while the unit tests are checked on different versions, the code formats are not. Trying to add build matrix to code format too.

It's interesting that linting gives you different results on different Python versions. Maybe we should simply use e.g. Python 3.8 as the lining standard? I guess that would make things easier. Let me know what you think.

Finally, thank you so much for the cool regex parser and everything else in this PR!

docs/conf.py Outdated Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
Co-authored-by: Yuanming Hu <yuanming-hu@users.noreply.github.com>
@taichi-dev taichi-dev deleted a comment from codecov bot Sep 22, 2020
@rexwangcc
Copy link
Member Author

The lint step worked totally fine on my local environment but failed due to a really bizarre error. It also indicates that while the unit tests are checked on different versions, the code formats are not. Trying to add build matrix to code format too.

It's interesting that linting gives you different results on different Python versions. Maybe we should simply use e.g. Python 3.8 as the lining standard? I guess that would make things easier. Let me know what you think.

Thanks for your prompt reply! Since the ultimate goal of having a code formater (and linter) is to maintain a consistent unified code format so we don't have to spend precious time discussing about code styles, I think simply using Python 3.8 is a good idea. (The only concern that developers don't have 3.8 locally is already addressed by the format server/action, so it won't be a problem)

If it sounds good to you, I will update the action to use Python3.8 only. (Not sure if we need to update the format server as well, since I remember that's about to be deprecated) In the meantime, I realize other PRs are having this problem too, but most of the format checks there are skipped since action will ignore format checks if taichi-gardener is in the reviewer list, I don't get the rationale behind that, but it would be great it we re-enbale running format checks on every PR.

@yuanming-hu
Copy link
Member

If it sounds good to you, I will update the action to use Python3.8 only.

That sounds good! Thanks, and let's use 3.8 only.

(Not sure if we need to update the format server as well, since I remember that's about to be deprecated)

I believe we will still use the current format server :-) It's working reasonably well.

In the meantime, I realize other PRs are having this problem too, but most of the format checks there are skipped since action will ignore format checks if taichi-gardener is in the reviewer list, I don't get the rationale behind that, but it would be great it we re-enbale running format checks on every PR.

Interesting... I guess the desired behavior here is to always check the code format, and only trigger format server if Taichi gardener is on the reviewer list. Is this the current behavior? IIRC the code format check is always on.

Linting is a different thing from code formatting. I believe currently there are too many linting warnings and it will take us a while to manually fix the linter warnings... So for now, we should not strictly enforce "no linting warnings", but after we clear up the warnings we should enforce that as well to make sure no new listing warnings are introduced.

@codecov
Copy link

codecov bot commented Sep 22, 2020

Codecov Report

Merging #1885 into master will increase coverage by 0.95%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1885      +/-   ##
==========================================
+ Coverage   43.19%   44.14%   +0.95%     
==========================================
  Files          44       44              
  Lines        6330     6121     -209     
  Branches     1092     1092              
==========================================
- Hits         2734     2702      -32     
+ Misses       3426     3250     -176     
+ Partials      170      169       -1     
Impacted Files Coverage Δ
python/taichi/lang/ast_checker.py 70.58% <0.00%> (-1.64%) ⬇️
python/taichi/testing.py 75.00% <0.00%> (-0.72%) ⬇️
python/taichi/lang/linalg.py 89.33% <0.00%> (-0.67%) ⬇️
python/taichi/lang/meta.py 62.31% <0.00%> (-0.54%) ⬇️
python/taichi/lang/__init__.py 41.94% <0.00%> (-0.51%) ⬇️
python/taichi/misc/util.py 17.48% <0.00%> (-0.26%) ⬇️
python/taichi/misc/task.py 0.00% <0.00%> (ø)
python/taichi/lang/shell.py 0.00% <0.00%> (ø)
python/taichi/tools/patterns.py 0.00% <0.00%> (ø)
python/taichi/lang/kernel.py 71.17% <0.00%> (+0.13%) ⬆️
... and 9 more

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 f2a798d...9cf3da2. Read the comment docs.

@rexwangcc
Copy link
Member Author

rexwangcc commented Sep 22, 2020

Interesting... I guess the desired behavior here is to always check the code format, and only trigger format server if Taichi gardener is on the reviewer list. Is this the current behavior? IIRC the code format check is always on.

Ah, I see, the desired behavior totally makes sense to me. I think my confusion is from this line:

if: ${{ !contains(github.event.pull_request.requested_reviewers.*.login, 'taichi-gardener') }}

and the fact that other PRs that have gardener on the list have skipped code checks. I also misunderstood the purpose of Taichi Gardener action 🤦

Anyways, I can try to understand the CI workflows more as I'm working on the release pipeline, and hopefully we can standardize/simplify the CI as a whole.

Copy link
Member

@yuanming-hu yuanming-hu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Looks great now.

@yuanming-hu yuanming-hu merged commit 7c560b2 into taichi-dev:master Sep 22, 2020
@yuanming-hu yuanming-hu mentioned this pull request Sep 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants