Boweny/single controller #114
Conversation
| # NOTE we have to keep all the MCT orchestrator started processes alive with this barrier | ||
| # until the ray cluster is stopped, otherwise the MCT orchestrator will reclaim the resources | ||
| # once the processes on a node exit | ||
| dist.barrier() |
There was a problem hiding this comment.
I don't think you want a barrier here if im understanding correctly, because then it will timeout once the dist timeout is hit
There was a problem hiding this comment.
indeed, didn't find any good way to enable unlimited timeout for barrier (torch has one that accepts arbitrary long timeout but still not simply unlimited), so added a comment that we may still need a SyncActor
There was a problem hiding this comment.
In that case, do we want to use the longest reasonable timeout until we add support for a SyncActor (instead of using the default timeout)?
There was a problem hiding this comment.
my plan is just to use SyncActor again in a following PR, it is a bit to define longest reasonable timeout, e.g., we can define it to 1 year or more, but that's a bit odd
|
|
||
| def init_model(self, model_name: str): | ||
| """Initialize the model.""" | ||
| self.model = AutoModelForCausalLM.from_pretrained( |
There was a problem hiding this comment.
i don't think you want torch_dtype auto, might end up with bf16 master weights. should always be float 32. i know this is just a test, just noting
There was a problem hiding this comment.
removed it and seems still working
bowenyang008
requested review from
bcui-db,
gupta-abhay,
abaheti95 and
jdchang1
as code owners
| Returns: | ||
| bool: True if Ray is setting CUDA_VISIBLE_DEVICES, False otherwise | ||
| """ | ||
| return os.environ.get( |
There was a problem hiding this comment.
Is this something we want to support or should we disable this feature entirely?
| os.environ['RANK'] = str(rank) | ||
|
|
||
| # Set LOCAL_RANK based on Ray GPU allocation | ||
| os.environ['LOCAL_RANK'] = '0' if is_cuda_visible_devices_set( |
There was a problem hiding this comment.
Seems that setting all nodes' LOCAL_RANK=0 would lead to lack of 2D parallelism compatibility down the line, right?
| self.master_port = master_port | ||
|
|
||
| # Set up basic environment variables | ||
| os.environ['WORLD_SIZE'] = str(world_size) |
There was a problem hiding this comment.
Would updating these env vars lead to issues with the base (gloo) process group defined here or is that process group no longer relevant?
| 'GPU': 1, | ||
| 'CPU': 1, | ||
| 'worker_node': 1, | ||
| }] * tensor_parallel_size * num_engines | ||
| 'worker_node': 0, |
There was a problem hiding this comment.
Curious what the impact of changing worker_node to 0 means in this case? I would assume it makes sense, but wanted to understand if you knew why it was 1 originally.
There was a problem hiding this comment.
ah, good catch, the default should be 1 and am allowing 0 basically for single node testing otherwise it is forcing allocating an entire node for inference engine
| def init_train_process_group(self): | ||
| """Initialize the distributed process group.""" | ||
| # Initialize process group | ||
| dist.init_process_group(timeout=timedelta(seconds=30)) |
There was a problem hiding this comment.
Also curious about the implications about using dist.init_process_group here and using the vllm_utils.py specific init_process_group for creating the vllm engine...
No description provided.