use an agent-id rather than the process PID #24968

mattip · 2022-05-19T16:32:00Z

Why are these changes needed?

When using ray inside a virtualenv on windows, python.exe as reported by sys.executable is a PEP397 launcher to the actual python as reported by os.getpid():

>>> import sys, os, psutil
>>> >>> print(sys.executable)
C:\temp\issue24361\Scripts\python.exe
>>> os.getpid()
2208
>>> child = psutil.Process(2208)
>>> child.cmdline()
['C:\\oss\\CPython38\\python.exe']
>>> child.parent().cmdline()
['C:\\temp\\issue24361\\Scripts\\python.exe']
>>> child.parent().pid
6424

When the agent_manager launches the agent process via Process::Process(), it gets the PID of the launcher process (6424), which is what is expected as an ID when registering the agent in the gRPC callback. But inside agent.py, the child process reports the PID via os.getpid(), which is 2208, and this is the wrong PID to register the agent.

The solution proposed here is another version of #24905 that creates a int agent_id = rand(); before starting the python process, and passes the agent_id to the process.

Related issue number

Closes #24361, #23945

Checks

I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

mattip · 2022-05-19T16:33:31Z

@rkooo567 this replaces the other PR, which I closed.

mattip · 2022-05-19T16:34:26Z

src/ray/raylet/agent_manager.cc

@@ -55,27 +55,34 @@ void AgentManager::StartAgent() {
    RAY_LOG(INFO) << "Not starting agent, the agent command is empty.";
    return;
  }


I moved the stanza below to after adding the new arguments so it prints out everything

rkooo567

This seems a lot better! I think we should do 2 additional things.

(1) Remove agent_pid from the gRPC request of RegisterAgentRequest since it is not pid anymore
(2)

ray/src/ray/raylet/agent_manager.cc

Line 34 in 0212f5e

agent_pid_ = request.agent_pid();

At this code, can you change the code to agent_id_ = request.agent_id();?

rkooo567 · 2022-05-20T02:06:45Z

src/ray/raylet/agent_manager.cc

-                             << " has not registered. ip " << agent_ip_address_
-                             << ", pid " << agent_pid_;
+        [this, child, agent_id]() mutable {
+          if (agent_pid_ != agent_id) {


Instead of agent_pid_, can we rename it to agent_id_? Since it is not pid anymore.

Done, and improved the error message

rkooo567 · 2022-05-20T02:07:54Z

dashboard/agent.py

@@ -54,6 +54,7 @@ def __init__(
        raylet_name=None,
        logging_params=None,
        disable_metrics_collection: bool = False,
+        agent_id: int = os.getpid(),


I think we don't need the default id as pid in this case. Can you remove it?

Refactored to use a required keyword-only * signature.

Hmm I might be missing something here, but it still seems to receive os.getpid() as default?

Sorry, check now :)

rkooo567 · 2022-05-20T02:09:29Z

src/ray/raylet/agent_manager.cc

-
-  // Launch the process to create the agent.
-  std::error_code ec;
+  // Create a random agent_id to pass to the child process


Suggested change

// Create a random agent_id to pass to the child process

// Create a random agent_id to pass to the child process. We cannot use pid an id because os.getpid() from the python process is not reliable in Windows. See [Github issue link]

mattip · 2022-05-20T06:52:22Z

I refactored all the naming and also tried to improve the error reporting.

rkooo567

Looks great!!

rkooo567 · 2022-05-20T07:27:05Z

dashboard/agent.py

@@ -54,6 +54,7 @@ def __init__(
        raylet_name=None,
        logging_params=None,
        disable_metrics_collection: bool = False,
+        agent_id: int = os.getpid(),


Hmm I might be missing something here, but it still seems to receive os.getpid() as default?

rkooo567 · 2022-05-20T16:53:35Z

Failed tests seem unrelated. @mattip can you verify test_args failure from Windows is unrelated to your PR?

jjyao · 2022-05-20T20:59:25Z

Looks like StartupToken for worker process :)

mattip · 2022-05-22T16:06:47Z

Can you link to the test_args failure? It looks to me like windows is passing...

rkooo567 · 2022-05-27T05:10:15Z

Oh I re-ran the test. It is okay to merge I think

rkooo567 · 2022-05-27T05:10:50Z

Thanks for addressing the comment :). Pretty happy for the final version of this fix!!

This reverts commit 02f220b.

@mattip

This reverts commit 02f220b.   ## Why are these changes needed? Looks like this commit makes `test_ray_shutdown` way more flaky. cc @mattip for further investigation after revert <img width="760" alt="Screen Shot 2022-05-31 at 11 14 48 PM" src="https://user-images.githubusercontent.com/18510752/171339737-f48e6e90-391a-4235-bfac-a0aa0e563eb7.png"> ## Related issue number  ## Checks - [ ] I've run `scripts/format.sh` to lint the changes in this PR. - [ ] I've included any doc changes needed for https://docs.ray.io/en/master/. - [ ] I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/ - Testing Strategy - [ ] Unit tests - [ ] Release tests - [ ] This PR is not tested :(

…ect#24968)" (ray-project#25342)" This reverts commit 3385d19.

#25376)

…24968)"… (#25376)" This reverts commit cb151d5.

…24968)"… (#25376)" (#25669) This reverts commit cb151d5.

Redo the agent-id changes from #24968. The original PR is in the first commit, the second commit fixes a fatal flaw when using RAY_BACKEND_LOG_LEVEL=debug, which caused the "Ray C++, Java" tests to fail on macOS.

Redo the agent-id changes from ray-project#24968. The original PR is in the first commit, the second commit fixes a fatal flaw when using RAY_BACKEND_LOG_LEVEL=debug, which caused the "Ray C++, Java" tests to fail on macOS. Signed-off-by: Rohan138 <[email protected]>

Redo the agent-id changes from ray-project#24968. The original PR is in the first commit, the second commit fixes a fatal flaw when using RAY_BACKEND_LOG_LEVEL=debug, which caused the "Ray C++, Java" tests to fail on macOS. Signed-off-by: Stefan van der Kleij <[email protected]>

mattip mentioned this pull request May 19, 2022

find agent_pid even when using a PEP397 launcher #24905

Closed

6 tasks

mattip commented May 19, 2022

View reviewed changes

use an agent-id rather than the process PID

4b4975e

mattip force-pushed the issue24361-take2 branch from 48d1209 to 4b4975e Compare May 19, 2022 16:43

scv119 assigned rkooo567 May 19, 2022

rkooo567 reviewed May 20, 2022

View reviewed changes

from review: improve error message and change PID -> ID

54d33aa

rkooo567 approved these changes May 20, 2022

View reviewed changes

from review: make some agent.py kwargs required and remove defaults

1e4c246

mattip mentioned this pull request May 26, 2022

Enable conda runtime creation in workers on windows #23613

Merged

6 tasks

rkooo567 merged commit 02f220b into ray-project:master May 27, 2022

DmitriGekhtman mentioned this pull request May 27, 2022

[Core] ray.exceptions.LocalRayletDiedError: The task's local raylet died. Check raylet.out for more information. #24361

Closed

This was referenced May 28, 2022

[Core] Error on Windows with virtual environment: gRPC unavailable #23945

Closed

[core/win] ray.init is hanging forever on windows (vscode) #12481

Closed

rkooo567 added a commit to rkooo567/ray that referenced this pull request Jun 1, 2022

Revert "use an agent-id rather than the process PID (ray-project#24968)"

30b0e70

This reverts commit 02f220b.

rkooo567 added a commit to rkooo567/ray that referenced this pull request Jun 1, 2022

Revert "Revert "use an agent-id rather than the process PID (ray-proj…

4ae315a

…ect#24968)" (ray-project#25342)" This reverts commit 3385d19.

fishbone pushed a commit that referenced this pull request Jun 1, 2022

Revert "Revert "use an agent-id rather than the process PID (#24968)"… (

cb151d5

#25376)

simon-mo added a commit that referenced this pull request Jun 10, 2022

Revert "Revert "Revert "use an agent-id rather than the process PID (#…

92b586d

…24968)"… (#25376)" This reverts commit cb151d5.

simon-mo added a commit that referenced this pull request Jun 13, 2022

Revert "Revert "Revert "use an agent-id rather than the process PID (#…

feb8c29

…24968)"… (#25376)" (#25669) This reverts commit cb151d5.

This was referenced Jun 15, 2022

Revert "Revert "Revert "use an agent-id rather than the process PID (#24968)"…" #25669

Merged

Redo agent-pid -> agent-id changes #25806

Merged

mattip mentioned this pull request Jul 26, 2023

[core][3/3] Use the new standalone runtime env http server. #37585

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use an agent-id rather than the process PID #24968

use an agent-id rather than the process PID #24968

mattip commented May 19, 2022 •

edited

Loading

mattip commented May 19, 2022

mattip May 19, 2022

rkooo567 left a comment

rkooo567 May 20, 2022

mattip May 20, 2022

rkooo567 May 20, 2022

mattip May 20, 2022

rkooo567 May 20, 2022

mattip May 20, 2022

rkooo567 May 20, 2022

mattip commented May 20, 2022

rkooo567 left a comment

rkooo567 May 20, 2022

rkooo567 commented May 20, 2022

jjyao commented May 20, 2022

mattip commented May 22, 2022

rkooo567 commented May 27, 2022

rkooo567 commented May 27, 2022

	// Create a random agent_id to pass to the child process
	// Create a random agent_id to pass to the child process. We cannot use pid an id because os.getpid() from the python process is not reliable in Windows. See [Github issue link]

use an agent-id rather than the process PID #24968

use an agent-id rather than the process PID #24968

Conversation

mattip commented May 19, 2022 • edited Loading

Why are these changes needed?

Related issue number

Checks

mattip commented May 19, 2022

Choose a reason for hiding this comment

rkooo567 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattip commented May 20, 2022

rkooo567 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkooo567 commented May 20, 2022

jjyao commented May 20, 2022

mattip commented May 22, 2022

rkooo567 commented May 27, 2022

rkooo567 commented May 27, 2022

mattip commented May 19, 2022 •

edited

Loading