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

[Testing] Move some tests from run_matmul_test.sh to run.py #885

Closed
wants to merge 3 commits into from

Conversation

newling
Copy link
Contributor

@newling newling commented Nov 7, 2024

I've tested that the logic for approximate testing is correct (and like IREE's) but I should probably add a unit test. Tricky though, need to inject a failure. If people believe strongly we should/will just never test matmuls with M*N*K >1e11 I can simplify this and scrap the sampling logic.

@newling
Copy link
Contributor Author

newling commented Nov 8, 2024

Damn I just another incomplete effort to unify the testing so I'm not sure what the best way to clean things up is now

#795

@newling
Copy link
Contributor Author

newling commented Nov 8, 2024

I think I/we should try the following approach (slow burn, expect many delays)

  1. move and eliminate run_matmul_test.sh by porting the tests to run.py
  2. move and eliminate run.py by porting the tests to Use python bindings for tests #795

Why not move all directly to #795 ? Because steps 1 and 2 are each smaller jumps and I think the metric to use for difficulty here is more l_inf than l_2

@jtuyls
Copy link
Collaborator

jtuyls commented Nov 18, 2024

@newling Is this PR meant to be reviewed or is the port of all tests a WIP? I am not sure based on your latest comment.

@newling
Copy link
Contributor Author

newling commented Nov 18, 2024

@newling Is this PR meant to be reviewed or is the port of all tests a WIP? I am not sure based on your latest comment.

I don't mind, if you have an opinion on the high-level plan feel free to post it here. So the question to answer is: Will moving the tests in run_matmul_test.sh to run.py improve the situation, and is it low/medium/high priority? I think "Yes" and "low".

Also, I'm going to remove the complexity of approximate testing in this PR. Until we actually run very large matmuls, it's not helping at all:

In [6]: %time np.matmul(np.random.rand(1024, 4096), np.random.rand(4096, 1024))
CPU times: user 417 ms, sys: 839 ms, total: 1.26 s
Wall time: 116 ms

which compared to compile time is insignificant.

@jtuyls
Copy link
Collaborator

jtuyls commented Nov 19, 2024

I don't mind, if you have an opinion on the high-level plan feel free to post it here. So the question to answer is: Will moving the tests in run_matmul_test.sh to run.py improve the situation, and is it low/medium/high priority? I think "Yes" and "low".

I agree, we should move the tests to run.py first, and I also think it's low priority. But all new tests should be added to run.py imo, like we have been doing lately already.

Also, I'm going to remove the complexity of approximate testing in this PR. Until we actually run very large matmuls, it's not helping at all:

Yes, makes sense.

@newling
Copy link
Contributor Author

newling commented Dec 2, 2024

Closing as rebasing will be more effort than redoing

@newling newling closed this Dec 2, 2024
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.

2 participants