Actually do we need to change this function at all? I feel we only need to change get_current_process_visible_accelerator_ids to consider CUDA_VISIBLE_DEVICES_ENV_VAR

What are the typical use cases for HIP_VISIBLE_DEVICES_ENV_VAR and CUDA_VISIBLE_DEVIES_ENV_VAR? It seems like it's possible to explicitly HIP_VISIBLE_DEVICES_ENV_VAR without calling this function. If we return CUDA_VISIBLE_DEVICES_ENV_VAR and allow explicit usage of HIP_VISBILE_DEVICES_ENV_VAR, it can introduce inconsistencies.

Could you elaborate. What will be the issue if this function always returns HIP_VISIBLE_DEVICES_ENV_VAR as before?

It looks like Ray is able to correctly identify the correct number of device in either case with your suggestion as show in the screenshot below. If other reviewers (@HollowMan6 @kevin85421) have no concerns, this function can always return HIP_VISIBLE_DEVICES_ENV_VAR

@jjyao is there anything else I can do to move this PR forward?

I think this function is supposed to be a pure getter function that doesn't modify os.environ.

At high level should it be:

def get_current_process_visible_accelerator_ids():

// decide which env var to use
env_var = HIP_VISIBLE_DEVICE
if CUDA_VISIBLE_DEVICE is not None:
   if HIP_VISIBLE_DEVICE is not None:
      // make sure the value is the same
   else:
      env_var = CUDA_VISIBLE_DEVICE
      
  return os.envion[env_var].split(",")

@vickytsang does the above comment make sense to you?

@jjyao yes thank you. Please standby. I will push an other commit in the next hour.

we can merge this with the existing test_amd_gpu.py

would you like the test_amd_gpu1.py to be removed from this PR?

Yes. Add the tests in test_amd_gpu1.py to test_amd_gpu.py. I think some tests can be combined with pytest.param()

This file can be removed

@vickytsang can you delete this file?

This can be combined with test_visible_amd_gpu_ids using pytest parametrization. You can do something like

@pytest.mark.parametrize("visible_devices_env_var", ("HIP_VISIBLE_DEVICES", "CUDA_VISIBLE_DEVICES"))
@patch(
    "ray._private.accelerators.AMDGPUAcceleratorManager.get_current_node_num_accelerators",  # noqa: E501
    return_value=4,
)
def test_visible_amd_gpu_ids(mock_get_num_accelerators, visible_devices_env_var, monkeypatch, shutdown_only):
    monkeypatch.setenv(visible_devices_env_var, "0,1,2")
    # Delete the cache so it can be re-populated the next time
    # we call get_accelerator_manager_for_resource
    del get_accelerator_manager_for_resource._resource_name_to_accelerator_manager
    ray.init()
    _ = mock_get_num_accelerators.called
    assert ray.available_resources()["GPU"] == 3

so you don't need to repeat most of the tests.

@jjyao Done. All CI tests are passing. Please review.