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
Add buildkite annotation when notebook parallelism setting is wrong #916
Conversation
Code Climate has analyzed commit 7067f61 and detected 0 issues on this pull request. View more on Code Climate. |
This reverts commit 0bc668d.
Codecov Report
@@ Coverage Diff @@
## develop #916 +/- ##
=======================================
Coverage 84.4% 84.4%
=======================================
Files 52 52
Lines 5080 5080
=======================================
Hits 4288 4288
Misses 792 792 Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## develop #916 +/- ##
=======================================
Coverage 84.4% 84.4%
=======================================
Files 52 52
Lines 5080 5080
=======================================
Hits 4288 4288
Misses 792 792
Continue to review full report at Codecov.
|
echo "Buildkite step parallelism is set to $SPLIT but found $NUM_NOTEBOOKS_TOTAL notebooks. Please update parallelism to match the number of notebooks!" | ||
msg="Buildkite step parallelism is set to $SPLIT but found $NUM_NOTEBOOKS_TOTAL notebooks. Please update the notebook testing \`parallelism\` setting in \`.buildkite/pipeline.yml\` to match the number of notebooks! | ||
|
||
If a pull request adding or removing notebooks was merged recently, this may be fixed by doing a merge with \`develop\`; for example: \`git fetch origin && git merge origin/develop\`." |
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.
just to clarify, when will merging with develop fix this?
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.
When a PR adding or removing notebooks was merged into develop
, and the current build's commit doesn't inherit from that merged commit, e.g.: if the git DAG for develop
and the PR in question looks like
D (develop)
|
| P (PR)
| /
B
|
older
|
commits
|
...
Suppose commit D
on develop
adds a notebook and so has parallelism: 40
, vs. the PR commit P
that has parallelism: 39
. When P
is built against D
, the pipeline.yml
will be uploaded from P
(and so parallelism: 39
). The notebook steps will be run after doing a transient merge with D
on the CI machines, the plugins
anchor pulls in the https://github.com/stellargraph/github-merged-pr-buildkite-plugin plugin:
stellargraph/.buildkite/pipeline.yml
Lines 82 to 89 in 913cf97
- label: ":python::book: test notebooks %n" | |
<<: *timeout | |
key: "test-notebooks" | |
depends_on: "runner-3_6" | |
parallelism: 36 | |
command: ".buildkite/steps/test-demo-notebooks.sh" | |
plugins: | |
<<: *plugins |
After doing the merge with D
, the code being tested for P
will have 40 notebooks (the merge will bring in that extra notebook), and thus the parallelism setting of 39 will be wrong. If an explicit merge is done and pushed to the PR, the parallelism in the pipeline.yml
will be correct (40):
M (PR)
/ |
D (develop)
| |
| P
| /
B
|
|
older
|
commits
|
...
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.
ah makes sense, I wasn't aware of the transient merge going on that would pull in extra notebooks from develop 👍
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.
looks good!
If the number of notebooks doesn't match the
parallelism
setting used for testing notebooks, all of the steps will fail, and one needs to go into them to deduce what the problem was, and then interpret the message. This PR (a) rewrites the message to be a bit clearer, including mentioning the common case of needing to merge with develop, and (b) adds it as an annotation at the top of the build for convenience.Example
https://buildkite.com/stellar/stellargraph-public/builds/1628