Skip to content

Conversation

thefacetakt
Copy link
Contributor

Currently, there is no way to specify the location of a memory region in the Python API. As a result, every memory region is assigned to the wildcard (*) location, which makes explicitly specifying the topology (via MC_CUSTOM_TOPO_JSON) has no effect.

This PR adds support for an optional location argument in the Python API.

Copy link
Collaborator

@stmatengss stmatengss left a comment

Choose a reason for hiding this comment

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

Could you add a unittest (testing different parameters) for the new register_memory API in transfer_engine_initiator_test.py?

@staryxchen
Copy link
Contributor

Could you add a unittest (testing different parameters) for the new register_memory API in transfer_engine_initiator_test.py?

Hi @thefacetakt
I think your PR is very necessary. Could you add some tests based on the review comments? If you are not available at the moment, I would be happy to help.

@thefacetakt
Copy link
Contributor Author

Hi @stmatengss

I've added a simple test which uses register_memory api with and without location argument.
Right now it basically tests that using this api does not break anything. It is challenging to test effect of providing location, as:

  1. tcp transport ignores location
  2. even with RDMA the effects are subtle and, i think, hard to test directly.

Did you have in mind that simple test, or do you have any ideas for more substantial test?

@stmatengss
Copy link
Collaborator

Hi @stmatengss

I've added a simple test which uses register_memory api with and without location argument. Right now it basically tests that using this api does not break anything. It is challenging to test effect of providing location, as:

  1. tcp transport ignores location
  2. even with RDMA the effects are subtle and, i think, hard to test directly.

Did you have in mind that simple test, or do you have any ideas for more substantial test?

A simple unit test is necessary (for both TCP and RDMA). You can try to add a unit test in memory_location_test.cpp. We aim to reduce parameters and ensure that any newly introduced parameter does not affect stability. THX!

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