-
Notifications
You must be signed in to change notification settings - Fork 29
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
Use python bindings for tests #795
Conversation
48830a5
to
c4ac2ba
Compare
c1bdeb0
to
df692aa
Compare
df692aa
to
12787ce
Compare
target_compile_definitions(${_core_lib} | ||
PUBLIC | ||
-DBOOST_BIND_GLOBAL_PLACEHOLDERS | ||
$<$<CONFIG:Debug>:-DXRT_VERBOSE>) |
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.
Is this for debugging purposes?
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.
Yes
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.
Should we enable it by default?
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.
Nah it's too "loud".
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.
Sorry, I was assuming it was enabled by default now. If it's not, that's good
Co-authored-by: Jorn Tuyls <[email protected]>
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
a936db5
to
fc48087
Compare
Off the top of my head things which we need to reach parity, hopefully we can reuse what's already done in cpu_comparison/run.py:
Also, can you/someone please at some point:
|
None have been moved, some have been duplicated. |
This PR begins the migration to
pytest
for all of oure2e
tests using IREE's python bindings for both the compiler and the runtime. I've tested it thoroughly (by runningrun_matmul.sh
andcpu_comparison/run.py
10050 times).Two major changes were required to make this happen:
AMDAIEOptions
so that they're registered at plugin load time. This was necessary for enablingSession.set_flags
in Python. Going forward we should make sure to put all CLI args in the same place;1536x2048x1536
that was prior disabled for Windows but is now disabled entirely. I don't know what the connection is (I wasn't able to track it down) but since every other test passes, including the 1000 run test, and even after repeating overall10050 runs, I think it's safe to conclude there's something wrong with that shape rather than the HAL.The migration isn't complete because the bindings and the VM itself don't support bf16 yet; I will send them a PR soon.
Note, in its current state the test scripts run
10050 times. I will remove this before landing.TODO (for the next iteration): right now
repeat_runs
doesn't work because of how the XRT command buffer works (it's free'ed after every dispatch and with it thexrt::kernel
andxrt::hw_context
). Likely I'm doing something wrong (possibly using the wrong command buffer model) so I think it should be fixable.