improve utils/check_bad_commit.py
#41658
Conversation
| raise ValueError(error_msg) | ||
| error_msg = f"Error when running git bisect:\nbash error: {result.stderr}\nbash output:\n{result.stdout}\nset `bad_commit` to `None`." | ||
| print(error_msg) | ||
| return None |
There was a problem hiding this comment.
Let's set it to None but not fail the whole script. We still want to continue to check the next failing test and find out which commit cause it.
Otherwise, we would not receive the reprot (the github action job using this script will fail)
There was a problem hiding this comment.
Fair enough, let's see how many of the error logs above we get 😄
There was a problem hiding this comment.
It's not much I believe, except those
DeprecationWarning: builtin type swigvarlink has no module attribute
being thought as error (before this PR), which is not.
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
vasqu
left a comment
vasqu
left a comment
There was a problem hiding this comment.
Thx, lgtm overall
Do we want this to be a permanent solution or is it a workaround for now? Let's see how it fares
| print("test failed") | ||
| exit(2) | ||
| exit(0) |
There was a problem hiding this comment.
Nit: would we add print("test passed") before here now
There was a problem hiding this comment.
you mean before exit(0)? Good point, it's not always passed, it could be SKIPPED or anything that is not FAILED and ERROR.
I will add a print.
| raise ValueError(error_msg) | ||
| error_msg = f"Error when running git bisect:\nbash error: {result.stderr}\nbash output:\n{result.stdout}\nset `bad_commit` to `None`." | ||
| print(error_msg) | ||
| return None |
There was a problem hiding this comment.
Fair enough, let's see how many of the error logs above we get 😄
Kind of, it is improved along the time: it's use Later, I will need to handle the flaky tests too to make the results more reliable. |
* fix * Update utils/check_bad_commit.py Co-authored-by: Anton Vlasjuk <[email protected]> * fix * fix * fix --------- Co-authored-by: ydshieh <[email protected]> Co-authored-by: Anton Vlasjuk <[email protected]>
* robust * robust * robust --------- Co-authored-by: ydshieh <[email protected]>
…ingface#41690) * fix * Update utils/check_bad_commit.py Co-authored-by: Anton Vlasjuk <[email protected]> * fix * fix * fix --------- Co-authored-by: ydshieh <[email protected]> Co-authored-by: Anton Vlasjuk <[email protected]>
…41815) * 111 * fix * fix * fix --------- Co-authored-by: ydshieh <[email protected]>
…e#41658) (huggingface#41815) * 111 * fix * fix * fix --------- Co-authored-by: ydshieh <[email protected]>
…e#41658) (huggingface#41815) * 111 * fix * fix * fix --------- Co-authored-by: ydshieh <[email protected]>
What does this PR do?
This script (used to find which commits fail certain failing tests on our CI) fails recently, due to the fact that the warning
sys:1: DeprecationWarning: builtin type swigvarlink has no __module__ attributewas written tostderrand we usestderrto check if there is error.This is not a good approach, we should instead check
result.returncode. Also a few improvements added along this here