Repository navigation
Conversation
github-actions
Bot
added
testing
fe9c5d9 to
cd53112
Compare
|
I opened a multi-GPU tensor-split issue that may be a useful real-world target for this scheduler/backend test work: The failure is strongly timing-sensitive and so far I have not been able to reduce it to a deterministic backend test. Setup is Windows + 2x RTX 3090 + Qwen 3.8 27B tensor split. I also reproduced a related failure on official upstream b10595 with MTP disabled. A few things make it interesting for this PR:
This is exactly the kind of bug where turning the timing relationship into a small deterministic scheduler/backend RED would be much more useful than continuing to reproduce it with 20k+ token model workloads. I can run specific synthetic tests if there is a scheduler/meta pattern you want exercised. |
|
@Anbeeld interesting, thanks for linking this. If you are curious and want to debug further in the mean time, I suggest disabling PDL ( |
3ef4094 to
7b48156
Compare
aendk
changed the title
JohannesGaessler
left a comment
JohannesGaessler
left a comment
There was a problem hiding this comment.
So far I've taken a look at everything except the Vulkan changes and test-backend-sched.cpp. I'm not sure I agree with the specification/implementation of GGML_OP_SLEEP. It has I think more complexity than necessary and that is already causing slight inconsistencies between the CPU and CUDA backend. The CPU backend determines some point in the future, does the memcpy, and then waits until the predetermined time arrives. The CUDA backend sleeps for a set duration and then does a memcpy after the time has arrived. I can imagine scenarios where some ggml backend writes to the original buffer in the meantime and causes inconsistent results.
My opinion is that GGML_OP_SLEEP should only cause a delay without any memcpys. This can be done by creating a view of the original tensor (see ggml_view_impl in ggml.c). A backend will then only need to worry about implementing the delay without touching any actual data. This would also enable the use ggml_sleep for non-contiguous tensors. For purposes of ggml_backend_set and ggml_backend_get there should be no need to change test-sleep.cpp although an additional check asserting that ggml_tensor::data is the same could be added.
|
I've now read through I don't feel confident in my Vulkan expertise so ideally those parts would be reviewed by one the corresponding maintainers. |
JohannesGaessler
left a comment
JohannesGaessler
left a comment
There was a problem hiding this comment.
Sorry, I tried to keep it short but it ended up being a lot anyways.
- When I say in this review that things are "not being tested" I do not mean that adding such tests is a prerequisite for a merge. If you add such test cases I will review them but it's also fine to only add TODO comments for now.
- Backend "promotion" like from CPU to GPU for better performance is not being tested.
- The meta backend is not being tested.
- Preferably use the RAII types like
ggml_backend_ptrdefined inggml-cpp.h. It's very easy to forget freeing memory. - Preferably use a prefix like
helper_rather thansched_to make it more clear that structures come from the tests rather than ggml. I don't feel strongly about this though. - I feel like in some parts of the code there is the intent to add "random" patterns like with
reach_backinstress_test_dag. Preferable use an actual RNG instead to make the intent more clear.
| // to later check if any further graph logic alters the number of splits | ||
| int n_splits_expected = g.nodes.empty() ? 0 : 1; | ||
| for (size_t i = 1; i < g.backend_id.size(); i++) { | ||
| n_splits_expected += g.backend_id[i] != g.backend_id[i - 1]; | ||
| } | ||
|
|
||
| ggml_backend_sched_reset(sched); | ||
| for (size_t i = 0; i < g.nodes.size(); i++) { | ||
| ggml_backend_sched_set_tensor_backend(sched, g.nodes[i], backends_w_caps[g.backend_id[i]].backend); | ||
| } |
There was a problem hiding this comment.
The way this is implemented sets an explicit backend for each tensor. However, one of the features of the ggml backend scheduler is that is automatically assigns backends where the user code does not do this. This feature is not covered by the tests.
There was a problem hiding this comment.
Correct, this is a required component to generate the specific schedules for the various scheduler-tests.
Have I understood you correctly that you would like me to add additional tests which primarily test that the auto-assign component of the scheduler works as intended?
| // GPU: increment both {99} and {66} -> {100} and {67} | ||
| // Correct result is thus {100}, incorrect output is {111} when {55} was overwritten by {66} before the copy started. | ||
| // Note: currently only reproducible on async H2D copy (=pinned memory). | ||
| static bool test_inputless_splits_scheduling(const sched_backend_caps & gpu, const sched_backend_caps & cpu, int64_t tensor_len, int32_t sleep_us, bool use_device_host_buft) { |
There was a problem hiding this comment.
For this test to work it is implicitly relying on certain patterns of reuse in the ggml graph allocator. If the graph allocator for whatever reason were to decide that the memory should not be re-used it will not be overwritten even if a race condition does occur. So I would suggest making the overwrite explicit by using ggml_add_inplace rather than ggml_add.
There was a problem hiding this comment.
I think this would break the test. This one was made specifically for the case where the allocator thinks it can safely reuse the memory, but the scheduler does not schedule the write->read->write->read pattern correctly so that it collapsed into write->write->read->read.
I can of course add a flag here to also test withggml_add_inplace, but I think it would help to keep at least one version of this test as is, also because last time I tested, #25319 fails this test.
|
@0cc4m if you have time, please take a look into the the vulkan parts. They are 100% vibed and I am certain that you have quality opinions on how to improve them 👨🍳 |
0cc4m
left a comment
0cc4m
left a comment
There was a problem hiding this comment.
I tested the Vulkan changes, looks good otherwise.
d013c05 to
9acd735
Compare
to toggle this behavior
test_chain_all_backends and test_pair_user_inputs. The latter ones test either activations or user inputs in isolation and just once, whereas the stress test tests all combinations multiple rounds
Co-authored-by: Johannes Gäßler <johannesg@5d6.de>
ggml_set_name/ggml_get_name
7a2261a to
7428801
Compare
transitions both with and without CPU input (next to activation copy as input)
Overview
This draft PR takes a first stab at formalizing the behavior of the scheduler and the async backends through test coverage.
There is lots to improve here, and I look forward to do so with your input. The main contribution is
test-backend-sched.cpp. To create more complex test-cases, I also added a new operatorOP_SLEEP(see below for details).I've implemented several classes of tests for this:
New OP_SLEEP
OP_SLEEPas opposed to some big MatMul for the following reasons:OP_SLEEPwas easy to implement in this scope (CPU, CUDA, VK)TODO
Requirements
cc @ggerganov @am17an @ORippler @gaugarg-nv