Better handle test-config labels on PR #122155
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/122155
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit b504f30 with merge base 666d629 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
huydhn
added
ciflow/mps
| print( | ||
| f"Select {config_name} because label {label} is presented in the pull request by the time the test starts" | ||
| ) | ||
| msg = f"Select {config_name} because label {label} is presented in the pull request by the time the test starts" |
There was a problem hiding this comment.
I know this is carried over from the previous, but I think it should be present in the pull request
|
|
||
| def has_labels(labels: Set[str], label_regex: Any) -> Set[str]: | ||
| """ | ||
| Return true if there is at least one label matching the regex |
There was a problem hiding this comment.
nit: filter_labels or get_labels might be better names for this function, also the doc string isn't right
| if not filtered_test_matrix["include"] and not valid_test_config_labels: | ||
| # Found no valid label and the filtered test matrix is empty, return the same | ||
| debug = re.compile(f"{PREFIX}.+") | ||
| print(type(debug)) |
| # Obviously, if the job name includes unstable, then this is an unstable job | ||
| is_unstable = job_name and IssueType.UNSTABLE.value in job_name | ||
| if not is_unstable and test_matrix: | ||
| if not is_unstable and test_matrix and test_matrix.get("include"): |
There was a problem hiding this comment.
Do you mind what the mps bug was? I don't understand how this fixes it
There was a problem hiding this comment.
I should have added a link to the description, but here it is https://github.com/pytorch/pytorch/actions/runs/8329872641/job/22802012305?pr=121381#step:9:151. So you can see the issue there in Is the current job unstable? True, this is wrong because there is no unstable issue for MPS. The bug is in this part is_unstable = all(IssueType.UNSTABLE.value in r for r in test_matrix["include"]) which returns True when test_matrix["include"] is an empty array.
Another way to express this is all(False for _ in []) returns True :)
|
@pytorchbot merge -f 'No need to run trunk, CI only fix' |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
I have some minor fixes in the scripts to 1. Fix the bug where the empty test matrix was confusingly print as unstable #121381 (comment) 1. Replace `print` with `logging.info` 1. Remove the hardcoded `VALID_TEST_CONFIG_LABELS` list. It's out of date and not many people use this features besides `test-config/default`, so why bother. The behavior here is simpler now: 1. If the PR has some `test-config/*` labels, they will be applied 1. If the PR has none of them, all test configs are applied 1. Add log for the previous 2 cases to avoid confusion ### Testing ``` python filter_test_configs.py --workflow "Mac MPS" --job-name "macos-12-py3-arm64 / build" --event-name "push" --schedule "" --branch "" --tag "ciflow/mps/121381" \ --pr-number 121065 \ --test-matrix "{ include: [ { config: "mps", shard: 1, num_shards: 1, runner: "macos-m1-stable" }, { config: "mps", shard: 1, num_shards: 1, runner: "macos-m2-14" }, ]} ``` Also running on this PR Pull Request resolved: #122155 Approved by: https://github.com/clee2000
3 participants
I have some minor fixes in the scripts to
printwithlogging.infoVALID_TEST_CONFIG_LABELSlist. It's out of date and not many people use this features besidestest-config/default, so why bother. The behavior here is simpler now:test-config/*labels, they will be appliedTesting
Also running on this PR