consolidate tests to use the fixture dask_client

[jnke2016]

consolidate tests to use the fixture dask_client

[jnke2016]

@xcadet I wanted to get your inputs on the changes I made with respect to some of the MG algos(mentioned in this PR) that use MGContext. In fact, I removed it so that all tests have their cluster and client initialized the same way through the fixture dask_client. I also removed MG_DEVICE_COUNT_OPTIONS because they are not very suited for MNMG. I have concerns with some of the tests which verify that everything works fine with and without the context manager setting up the cluster and the client. In fact, a test like test_enable_batch_view_then_context no longer make sense since the cluster and client are always initialized before the view and I do not find a way to go around it. I also renamed some of the tests because they no longer use a context. Some tests like test_enable_batch_no_context require the client or the comms to be None so I move those tests to the top before the fixture initializing the cluster and client is called. Do you have any suggestions?

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.10@7565dc3). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head cc17b7a differs from pull request most recent head 7014ed5. Consider uploading reports for the commit 7014ed5 to get more accurate results

@@               Coverage Diff               @@
##             branch-21.10    #1758   +/-   ##
===============================================
  Coverage                ?   59.60%           
===============================================
  Files                   ?       77           
  Lines                   ?     3533           
  Branches                ?        0           
===============================================
  Hits                    ?     2106           
  Misses                  ?     1427           
  Partials                ?        0           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7565dc3...7014ed5. Read the comment docs.

[xcadet]

Hello @jnke2016 , thank you for reaching out.

1. I am not sure I got the part about the MG_DEVICE_COUNT_OPTIONS removal, will setups with 1-2-3-4 GPUs still be tested?

   • For the 1 GPU G.enable_batch() is supposed to raise an exeception if there is no Dask Client / Comms,
     but should still work with a Single GPU if both the Dask Client and Comms are ready.
   • For the 2 GPUs it's the first actual MG setup
   • For the 3 GPUs it's the first odd number of GPUs MG tested (checks data distribution)
   • 4 GPUs it the first even number MG slightly more complex than 2 (checks that data distribution is properly handled as well)
   • The goal was not to have a really tight control of the GPU availability but more to ensure that these 4 core cases were handled properly.

2. If I got it right the fixture dask_client is defined with scope="session" and used across multiple algorithms (PR / Degree / BFS / ...).
   With scope="session" from my understanding, the set up and tear down are done once over the entire pytest run, therefore,
   even if you move the test_enable_batch_no_dask_client to the top of the test file, the dask_client maybe have already been setup anyway.
   With scope="module" the set up and tear down should only happen in a test file that requires it.
   Thus, the Dask Client would be created / destroyed once for every file requiring it.

   Is seems that pytest calls fixtures in the module only the first time they are required, therefore you could put all the tests that required both states while testing (No MG / MG) at the top of the file and use a "delayed setup".
   The "delayed setup" would be to rewrite the dask_client fixture in the same fashion as the MGContext (i.e. Using a Context Manager)
   The fixture could then be condensed as:

   @pytest.fixture(scope="module")
   def dask_client():
   	dask_scheduler_file = os.environ.get("SCHEDULER_FILE")
   	with DaskClient(dask_scheduler_file) as context:
   		yield context.client
   		print("\ndask_client fixture: client.close() called")
   		
   

   Then for the tests where you need to "delay" the setup you could still use the DaskClient as a Context Manager.

   def test_with_delay():
   	with DaskClient("CONTEXT") as context:
   		print("Test using (Delayed Setup)")
   

   By doing so you could have an unified way to setup MG tests while keeping the scoped fixture.
   Here is an example with pytest using scope="module", First / Second refer to their order in the test file:

   collected 5 items                                                                                                                                                                                                                                                                                         
   
   tests/a_test.py 
   [Contex started] FIXTURE
   First test to be executed (Requires Fixture)
   .
   [Context ended] FIXTURE
   
   tests/test_fixtures.py 
   First test to be executed (NO Fixture)
   .
   [Contex started] CONTEXT
   
   Second test to be executed (Delayed Setup)
   
   [Context ended] CONTEXT
   .
   [Contex started] FIXTURE
   
   Third test to be executed (Requires Fixture)
   .
   Fourth test to be executed (Requires Fixture)
   .
   [Context ended] FIXTURE
   

   The following is using scope="session", as you can see the fixture is invoqued once because the first test file required it:

   collected 5 items                                                                                                                                                                                                                                                                                         
   
   [Contex started] FIXTURE
   First test to be executed (Requires Fixture)
   .
   tests/test_fixtures.py 
   First test to be executed (NO Fixture)
   .
   [Contex started] CONTEXT
   
   Second test to be executed (Delayed Setup)
   
   [Context ended] CONTEXT
   .
   Third test to be executed (Requires Fixture)
   .
   Fourth test to be executed (Requires Fixture)
   .
   [Context ended] FIXTURE
   

I hope this is helpful.

[jnke2016]

Hello @xcadet I really appreciate the feedback and this is really helpful.
Regarding this

even if you move the test_enable_batch_no_dask_client to the top of the test file, the dask_client maybe have already been setup anyway

The dask_client has not already been setup until you call it in one of the test and I verified that by checking client and comms which return None.

regarding this

With scope="module" the set up and tear down should only happen in a test file that requires it.
Thus, the Dask Client would be created / destroyed once for every file requiring it

it is a good idea but the downside of it will be the setup and tear down for every tests which can be time consuming.

This

Is seems that pytest calls fixtures in the module only the first time they are required, therefore you could put all the tests that required both states while testing (No MG / MG) at the top of the file and use a "delayed setup".

is a great idea and I am currently exploring this. However, we will somehow have to run the tests (with the delayed setup) before any other tests because the first test that will setup the dask_client will do it for all the other tests.

[jnke2016]

What I meant in my last sentence with respect to your example (scope="session") is by the time we reach to this test file(test_mg_replication), there are prior test file (like test_renumbering) which already setup the dask_client

[jnke2016]

Hi @xcadet. I updated the conftest file and MGContext. I also kept the MG_DEVICE_COUNT_OPTIONS since they hold a purpose(which I didn't get until you explained it). Please let me know if you have something else to add.

Thank you

[BradReesWork]

@gpucibot merge