- 
                Notifications
    You must be signed in to change notification settings 
- Fork 37
Use dynamic ports in zmq tests #170
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
Conversation
Signed-off-by: Qifan Deng <[email protected]>
| Looking at https://libzmq.readthedocs.io/en/zeromq4-1/zmq_tcp.html, it looks like the address to start should be  | 
| 
 
 Among the combinations of host [ | 
Signed-off-by: Qifan Deng <[email protected]>
| The tests fail in https://github.com/pancak3/llm-d-inference-sim/blob/fab7019849fdcdfba989d4f66b3ecd230b53e9d6/pkg/kv-cache/kv_cache_test.go#L421 | 
Signed-off-by: Qifan Deng <[email protected]>
Signed-off-by: Qifan Deng <[email protected]>
Signed-off-by: Qifan Deng <[email protected]>
| @irar2 Not sure whether the change matches the above comment. I think I am not very familiar with the business logic of the KVEvent | 
        
          
                pkg/kv-cache/kv_cache_sender.go
              
                Outdated
          
        
      | DataParallelRank: &dpRank, | ||
| } | ||
|  | ||
| if s.publisher == nil { | 
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.
This check should be in Run() in two cases:
case eventData, ok := <-s.eventChan:
after if !ok{} please add in line 90
if s.publisher == nil { continue } 
and
case <-timer.C:
please add the same check first thing in this case.
I don't think we need a log message here, there can be too many.
Signed-off-by: Qifan Deng <[email protected]>
Signed-off-by: Qifan Deng <[email protected]>
| @pancak3 Thanks! /lgtm | 
resolve #164