-
Notifications
You must be signed in to change notification settings - Fork 234
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
[Windows] Support CPU shared memory (Client/Frontend) #551
base: main
Are you sure you want to change the base?
Conversation
src/python/library/tritonclient/utils/shared_memory/shared_memory_handle.h
Outdated
Show resolved
Hide resolved
#ifdef __cplusplus | ||
extern "C" { | ||
#endif | ||
|
||
#ifdef _WIN32 |
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.
Using a platform specific sub-struct with either HANDLE or int would clean up this logic as well.
Platform specific logic will be encapsulated within the sub-struct definition.
However, we must take care that we maintain client side backwards compatibility.
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 is handled opaquely now through use of a void
pointer, as discussed, and is used to initialize the appropriate type when creating the substruct. None of the client APIs needed modifications in our L0_shared_memory test, which gives me confidence that we did not break backwards compatibility with this change. Please let me know if you have other concerns.
platform_package_data += ["libcshm.so"] | ||
elif PLATFORM_FLAG == "win_amd64": | ||
platform_package_data += ["cshm.dll"] |
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.
Should we add an else block catching unsupported platform flags?
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.
The platform flag is currently passed in from build_wheel.py
, so, from our perspective, the possible platforms are all handled. If the platform does not contain the substring "linux" or is not "win_amd64", it will be "any". In this case, we do not include either shared library object.
Are you suggesting raising an error in the event the setup.py
script is called independently? Like so:
if "linux" in PLATFORM_FLAG:
platform_package_data += ["libcshm.so"]
elif PLATFORM_FLAG == "win_amd64":
platform_package_data += ["cshm.dll"]
elif PLATFORM_FLAG != "any":
raise Exception(f"Unsupported platform flag \"{PLATFORM_FLAG}\" specified.")
fcee735
to
479dccc
Compare
479dccc
to
43700e2
Compare
43700e2
to
f854d4a
Compare
Goal: Support CPU shared memory between the server and client for Windows
Server changes: triton-inference-server/server#7048