[Misc] Bound NIXL upper bound version#35495
Conversation
Signed-off-by: NickLucche <nlucches@redhat.com>
There was a problem hiding this comment.
Code Review
This pull request temporarily bounds the upper version of the nixl dependency to <0.10.0 to mitigate an agent disconnection issue. The change is correct and addresses the problem described. I've added one suggestion to include a comment in the requirements file explaining the reason for this version constraint. This will improve long-term maintainability by providing context for future dependency updates.
| @@ -1,3 +1,3 @@ | |||
| lmcache >= 0.3.9 | |||
| nixl >= 0.7.1 # Required for disaggregated prefill | |||
| nixl >= 0.7.1, < 0.10.0 # Required for disaggregated prefill | |||
There was a problem hiding this comment.
To improve maintainability, it would be helpful to add a brief comment explaining why the upper bound for nixl is necessary. This will provide context for future developers who might consider updating this dependency, preventing the reintroduction of the bug.
For example, you could mention the nixlRemoteDisconnectError bug mentioned in the pull request description.
nixl >= 0.7.1, < 0.10.0 # Required for disaggregated prefill. Bounded to avoid nixlRemoteDisconnectError.
There was a problem hiding this comment.
Agree with the boit, context would be good for maintainability pov.
There was a problem hiding this comment.
Discussed offline with @NickLucche , comment will be added when the issue is confirmed.
There was a problem hiding this comment.
Thanks @NickLucche for the PR. Looks good but agree with bot on comment for context.
Signed-off-by: NickLucche <nlucches@redhat.com>
Signed-off-by: NickLucche <nlucches@redhat.com>
Signed-off-by: NickLucche <nlucches@redhat.com>
Signed-off-by: NickLucche <nlucches@redhat.com>
Signed-off-by: NickLucche <nlucches@redhat.com>
Signed-off-by: NickLucche <nlucches@redhat.com>
Temporarily bounding NIXL to <0.10.0 after some internal reports of agent disconnection (nixlRemoteDisconnectError) by @ZhanqiuHu.
All in all, we should still have a bit more control over deps and add an upper version bound.