Skip to content
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

[RLlib] - Example _set_state in tcp_client_inference_env_runner incorrectly handles TemporaryDirectory (path issue). #50071

Open
zhujqlibiao opened this issue Jan 26, 2025 · 0 comments
Assignees
Labels
bug Something that is supposed to be working; but isn't rllib RLlib related issues rllib-client-server Issue related to RLlib's client/server API. rllib-newstack

Comments

@zhujqlibiao
Copy link

What happened + What you expected to happen

ray\rllib\env\tcp_client_inference_env_runner.py

The _set_state function in the RLlib example improperly uses TemporaryDirectory. Specifically:

The ONNX file is written to the current working directory instead of the temporary directory created by TemporaryDirectory.
This causes permission issues in environments where the current working directory is not writable.

Steps to Reproduce:
def _set_state(msg_body):
with tempfile.TemporaryDirectory():
with open("_temp_onnx", "wb") as f:
f.write(
gzip.decompress(
base64.b64decode(msg_body["onnx_file"].encode("utf-8"))
)
)
onnx_session = onnxruntime.InferenceSession("_temp_onnx")
output_names = [o.name for o in onnx_session.get_outputs()]
return onnx_session, output_names
Run the above code in an environment where the current working directory is not writable.
Observe the permission error.

Versions / Dependencies

Version: 2.41.0
windows11

Reproduction script

Problem Analysis:
The TemporaryDirectory is created but not used to store the file _temp_onnx. Instead, the file is written to the current working directory (os.getcwd()), which can cause issues in environments where the working directory is read-only or restricted.

Proposed Solution:
The ONNX file should be written to the temporary directory instead of the current working directory. Here's the corrected code:

def _set_state(msg_body):
with tempfile.TemporaryDirectory() as temp_dir:
temp_file_path = os.path.join(temp_dir, "_temp_onnx")
with open(temp_file_path, "wb") as f:
f.write(
gzip.decompress(
base64.b64decode(msg_body["onnx_file"].encode("utf-8"))
)
)
onnx_session = onnxruntime.InferenceSession(temp_file_path)
output_names = [o.name for o in onnx_session.get_outputs()]
return onnx_session, output_names
Why This Fix Works:
By explicitly using the path provided by TemporaryDirectory, the ONNX file is stored in a writable location that is automatically cleaned up after use.
This avoids permission issues and ensures the file is written safely in environments with restricted working directory permissions.

Issue Severity

None

@zhujqlibiao zhujqlibiao added bug Something that is supposed to be working; but isn't triage Needs triage (eg: priority, bug/not-bug, and owning component) labels Jan 26, 2025
@simonsays1980 simonsays1980 added rllib RLlib related issues rllib-client-server Issue related to RLlib's client/server API. rllib-newstack and removed triage Needs triage (eg: priority, bug/not-bug, and owning component) labels Jan 27, 2025
@simonsays1980 simonsays1980 changed the title [<Ray component: Core|RLlib|tcp_client_inference_env_runner>] Example _set_state incorrectly handles TemporaryDirectory (path issue) [RLlib] - Example _set_state in tcp_client_inference_env_runner incorrectly handles TemporaryDirectory (path issue). Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that is supposed to be working; but isn't rllib RLlib related issues rllib-client-server Issue related to RLlib's client/server API. rllib-newstack
Projects
None yet
Development

No branches or pull requests

3 participants