Skip to content
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

Merged
merged 19 commits into from
Oct 1, 2024
Merged

Conversation

makslevental
Copy link
Collaborator

@makslevental makslevental commented Sep 23, 2024

This PR begins the migration to pytest for all of our e2e tests using IREE's python bindings for both the compiler and the runtime. I've tested it thoroughly (by running run_matmul.sh and cpu_comparison/run.py 10050 times).

Two major changes were required to make this happen:

  1. Move all CLI args to AMDAIEOptions so that they're registered at plugin load time. This was necessary for enabling Session.set_flags in Python. Going forward we should make sure to put all CLI args in the same place;
  2. The XRT HAL had to be drastically refactored because in its current state (prior to this PR), it doesn't support multiple dispatches in a single process - I had to track down all kinds of resource leaks (why this PR took so much longer than I thought);
    • There's a test 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 overall 10050 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 the xrt::kernel and xrt::hw_context). Likely I'm doing something wrong (possibly using the wrong command buffer model) so I think it should be fixable.

@makslevental makslevental force-pushed the makslevental/python-bindings-tests branch 30 times, most recently from 48830a5 to c4ac2ba Compare September 28, 2024 01:06
@makslevental makslevental force-pushed the makslevental/python-bindings-tests branch from c1bdeb0 to df692aa Compare October 1, 2024 11:19
@makslevental makslevental force-pushed the makslevental/python-bindings-tests branch from df692aa to 12787ce Compare October 1, 2024 11:22
target_compile_definitions(${_core_lib}
PUBLIC
-DBOOST_BIND_GLOBAL_PLACEHOLDERS
$<$<CONFIG:Debug>:-DXRT_VERBOSE>)
Copy link
Collaborator

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link
Collaborator

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?

Copy link
Collaborator Author

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".

Copy link
Collaborator

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 <jtuyls@users.noreply.github.com>
Copy link
Collaborator

@jtuyls jtuyls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@makslevental makslevental force-pushed the makslevental/python-bindings-tests branch from a936db5 to fc48087 Compare October 1, 2024 12:50
@makslevental makslevental merged commit 59b5507 into main Oct 1, 2024
6 checks passed
@makslevental makslevental deleted the makslevental/python-bindings-tests branch October 1, 2024 13:42
@newling
Copy link
Contributor

newling commented Oct 1, 2024

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:

  • When the numerics fail, give good feedback
  • Random seed control
  • Only sample for very large matmuls, don't perform full CPU computation (maybe not needed)

Also, can you/someone please at some point:

  • document in the README how to run the pytests in the test dir
  • create an issue describing what work still needs to be done
  • keep track of migration of tests from old script to pytest, ideally they shouldn't be run in both places. Or maybe they've all been moved already?

@makslevental
Copy link
Collaborator Author

keep track of migration of tests from old script to pytest, ideally they shouldn't be run in both places. Or maybe they've all been moved already?

None have been moved, some have been duplicated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants