-
Notifications
You must be signed in to change notification settings - Fork 192
Add unit tests for multiple backends in a single agent #1049
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
Open
benlwalker
wants to merge
3
commits into
ai-dynamo:main
Choose a base branch
from
benlwalker:ben/multi-backend-tests
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
👋 Hi benlwalker! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
Verify that multiple backends can be created in a unit test. Signed-off-by: Ben Walker <[email protected]>
Test the case where two backends support the same memory type and the case where each backend supports different memory types. Signed-off-by: Ben Walker <[email protected]>
different backends This test fails. This test creates two backends that support DRAM. Then it does memory registration with one backend on one data buffer and memory registration on the other backend with a different data buffer. Then it tries to do prepXferDlist on a descriptor for both data buffers and that fails. This could be fixed by having prepXferDlist attempt to populate each segment with each backend and allowing for failure at the backend call without failing the entire operation. However, this just kicks the can down the road, because now the user can get failures in makeXferReq depending on which indices of the nixlXferDlistH they choose. I suggest fixing this with a different approach. First, we observe that almost all real users only have one backend type anyway. Second, if some user really has two separate pools of DRAM that are used by different backends with NIXL, they could just as easily solve that problem by creating two agents, each with one backend, rather than creating multiple backends. So I propose we make the agent require all backends to support all memory of the same type. In other words, do not allow registerMem to register on specific backends. Instead, registerMem should force the registration on all backends that support the memory type. Then the end user knows that for a given agent, all backends that can access DRAM can interoperate via all registered DRAM, for example. Even with the above change, NIXL can still support memory registered with different backends that don't overlap. The user would just need to use multiple agents. I think that's far more intuitive anyway. Signed-off-by: Ben Walker <[email protected]>
7257138 to
a5a4075
Compare
ColinNV
reviewed
Nov 27, 2025
| std::unique_ptr<char[]> buf2 = std::make_unique<char[]>(buf_size); | ||
|
|
||
| // Explicitly set addresses for the two DRAM blobs | ||
| uintptr_t addr1 = reinterpret_cast<uintptr_t>(buf1.get()); |
Contributor
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.
These and a couple of other variables can be made const.
Contributor
|
Looks clean to me; didn't dive into the logic of the tests. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Add GTest unit tests for several scenarios with multiple backends
Why?
From code inspection, I had a lot of questions about the exact behavior of many API calls when multiple backends are present. I wrote unit tests to clarify the behavior.
How?
I added some unit tests to the existing framework.