-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Improve superpmi-asmdiffs AzDO pipeline robustness #61819
Improve superpmi-asmdiffs AzDO pipeline robustness #61819
Conversation
1. When git fetching origin/main, use `--depth=500` to try to ensure there is enough context to allow finding a JIT change in the history. 2. Add some error checking in pipeline setup so failures in setting up the pipeline should fail the jobs early.
Tagging subscribers to this area: @JulieLeeMSFT Issue Details
|
@kunalspathak @dotnet/jit-contrib PTAL |
@@ -167,18 +171,22 @@ def main(main_args): | |||
os.makedirs(base_jit_directory) | |||
|
|||
print("Fetching history of `main` branch so we can find the baseline JIT") | |||
run_command(["git", "fetch", "origin", "main"], source_directory, _exit_on_fail=True) | |||
run_command(["git", "fetch", "--depth=500", "origin", "main"], source_directory, _exit_on_fail=True) |
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.
why this change?
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.
This fixes the problem we were seeing not finding a baseline JIT, e.g., https://dev.azure.com/dnceng/public/_build/results?buildId=1475182&view=logs&j=011e1ec8-6569-5e69-4f06-baf193d1351e&t=bf6cf4cf-6432-59cf-d384-6b3bcf32ede2.
The pipelines only fetch with depth=20
. If there aren't any JIT changes in those 20, we wouldn't find a baseline JIT. So, fetching a lot more makes it more likely to find a baseline JIT change.
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.
E.g., you can see the initial fetch
here:
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.
My impression was that git fetch origin main
(without --depth
argument) would fetch the entire history. Doesn't how it work?
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.
Well, I'm no expert, but it appears the answer is "no". It appears that the first fetch possibly creates a "shallow" repo and you either need to expand that by fetching with a larger depth, or use fetch --unshallow
to expand it. E.g., from the fetch
docs:
--depth=<depth> Limit fetching to the specified number of commits from the tip of each remote branch history. If fetching to a shallow repository created by git clone with --depth=<depth> option (see git-clone(1)), deepen or shorten the history to the specified number of commits. Tags for the deepened commits are not fetched.
"download", | ||
"-arch", arch, | ||
"-target_dir", base_jit_directory], | ||
source_directory) | ||
if return_code != 0: |
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.
You need to make sure that you return the actual error_code
from jitrollingbuild.py
file. Currently it always return success
.
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.
It turns out that doesn't matter: jitrollingbuild.py error model currently is to throw an exception, and that leads to a process error code of 1
. I started changing jitrollingbuild.py, then realized it wasn't necessary.
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.
Got it. Can you remove the unused success = True
from the jitrollingbuild.py
then?
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.
Done
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.
LGTM
--depth=500
to try to ensurethere is enough context to allow finding a JIT change in the history.
up the pipeline should fail the jobs early.