Repository navigation
Enable determinism check in CI - #85
Conversation
1b425b6 to
6df498b
Compare
…d in the benchmarking result artifact. Separate jobs for each arch will then check the results. If they fail, the email will that inevitable arrives after the job completes will contain something like "some of the jobs failed", which is good enough indicator for warranting a closer look.
ca0160f to
37fffbd
Compare
lauri9
left a comment
lauri9
left a comment
There was a problem hiding this comment.
CI_RUN_PY_FORCE_DETERMINISM_CHECK vs CI_RUN_PY_FORCE_DETERMINISM_REPORT - what is the difference and why do we need both?
Are the formatting changes necessary in run.py, they make reviewing a bit cumbersome
Ah, that's likely now redundant after I changed the approach. Good catch.
Pre-commit hook doing its thing. I'll bring the old version back and slap the changes on that since there aren't that many, but that reformatting should be done eventually. |
Sure, I'd be happy to approve a linting only PR since there it's clear that no functional changes are being introduced |
I'll put it on my list, needs some checking beforehand that nobody else is working on those files because in any of their own branches or it triggers a cornucopia of conflicts that are a huge pain to resolve manually. |
@lauri9 , |
| import os | ||
| from pathlib import Path | ||
|
|
||
| REPORT_FILENAME = "determinism_report.json" |
There was a problem hiding this comment.
can you have a single definition for that?
There was a problem hiding this comment.
Architecturally, the code that works with the determinism check results is spread across the run.py and this file and they became tightly coupled together. Can you maybe instead refactor the machinery in a single file that generates the report for run.py and provide what's needed for this script, so everything that works with the report itself is neatly encapsulated into a single module?
lauri9
left a comment
lauri9
left a comment
There was a problem hiding this comment.
Makes sense and statuses unavailable and error passing with warnings is okay. The core functionality seems to do what it's advertised to do based on the CI run. Consider if the default should be enabled, but this looks good to me. I suggest to wait for @Arech8 to also approve.
| return entry | ||
|
|
||
| if 1 != len(det_check): | ||
| logger.error("Expected exactly one top-level directory in results report") |
There was a problem hiding this comment.
this branch also needs entry["status"] = "error"
Instead of setting it all the time, I'd suggest making it the default value and set entry["status"] = "unavailable" under if determinism_check_results is None:
|
I got an idea that requires some bigger rewriting but keeps run.py much cleaner, functionally everything stays the same. |
…eterminism report script in more of a raw data handling state
|
So what changed is that now the determinism report handling is a more monolithic, separate entity and it handles the data side of things. The interpretation and actions inferred from that report is now handled by the separate, arch-specific (i.e. if there's runs for both mi300 and mi355, both will have their determinism report check jobs) job in the workflow. If the whole report is missing -> job fails Job failure does not prevent the whole workflow from finishing, but it will be noted in the email that lands in the user's inbox. |
Arech8
left a comment
Arech8
left a comment
There was a problem hiding this comment.
Great job, Joni 👍
Thanks a ton!
3 participants
Enable option to run determinism checks in CI.
Closes #84
Background
Determinism checks were introduced in run.py a short while ago, but there was no support in CI for it.
Goals
Allow running determinism checks while doing benchmarking.
Tasks
Tests
Other
This will trigger a run failed email with "Some jobs were not successful", and the email body shows what failed.