[core] If there's working_dir, don't set _py_driver_sys_path. #43214

rynewang · 2024-02-16T00:25:46Z

In #31383 we add _py_driver_sys_path to all workers, making them search from the driver's path. This makes the workers able to find modules near the driver file. However, in these cases it's not working:

if there's working_dir. The code should search from working_dir, not from the driver's dir. e.g. lib code in driver's dir can update after job submission and before worker starts, making the runs inconsistent among workers.
if the worker is in a different node than the driver's node. The code would search "the driver's dir as in the node where the driver runs in", but in a different node. If there happen to be a stale code file in the worker node, the stale code is used.
(already handled) if it's dashboard, it's client mode, or it's interactive mode.

This PR addresses the first issue. Left a TODO for the second issue.

Part of #42863

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rkooo567 · 2024-02-16T15:54:00Z

ETA for review this sun

rkooo567

Do we have an associated issue?

Also, this happens only when a driver is submitted to a worker node?

rkooo567 · 2024-02-20T10:42:00Z

python/ray/_private/worker.py

+        # (5) the driver is at the same node (machine) as the worker.
+        #
+        # We only do the first 4 checks here.
+        # TODO: do the (5) check by also passing node id.


why don't we just do it here?

We want to compare node_id of the driver vs node_id of this worker. We don't have the former value IIUC.

Hmm I prefer to just fix 100% in this PR rather than leaving it as tech debt if the fix is easy. Isn't this pretty easy to do it? (just add driver node id to job config?)

rkooo567 · 2024-02-20T10:42:22Z

python/ray/_private/worker.py

-            namespace and namespace == ray_constants.RAY_INTERNAL_DASHBOARD_NAMESPACE
+        # paths of the workers. Also add the current directory.
+        #
+        # Note that this is only meaningful when all of these are true:


can you also mention why here briefly (to the code comment)?

rkooo567 · 2024-02-20T10:47:18Z

python/ray/_private/worker.py

        runtime_env.pop("excludes", None)
        job_config.set_runtime_env(runtime_env)

    if mode == SCRIPT_MODE:


Btw, this code path is added only when the process is a driver. What's the reason why it is set for worker processes?

so the driver sets the _py_driver_sys_path here to record the driver's current path, then the worker adds it to its search path in maybe_initialize_job_config. If this worker is not a driver there's no point setting that.

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang · 2024-02-22T02:36:14Z

Test (basic_3.py) passing. There are some failures in Serve docs which should be irrevant.

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

…ath-workingdir Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

… to work because we checked the local dir, defying the point of the whole test. Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rkooo567

Changes look good, but I'd like to discuss a little bit more about #43214 (comment) before merging it.

…init. Added tests Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang · 2024-02-27T01:10:55Z

Added the 5th check, and a unit test. It should work now, @rkooo567

rkooo567 · 2024-02-27T13:49:54Z

python/ray/_private/worker.py

+        Get the driver node id via Job Info. Populates the cached driver node id if
+        found. The fetching can be slow if there are too many jobs.
+
+        @raises Exception if the gcs client times out.


Can you use the consistent docstring format as other functions here?

Args: Raises: Returns:

rkooo567 · 2024-02-27T13:50:46Z

python/ray/_private/worker.py

+    def get_driver_node_id(self, timeout=None) -> ray.NodeID:
+        """
+        Get the driver node id via Job Info. Populates the cached driver node id if
+        found. The fetching can be slow if there are too many jobs.


Actually fetching can be slow if there are so many workers started at the same time because GCS main thread is overloaded. I've seen it is delayed up to 10~30 seconds.

Ideally, we should not query GCS when a new worker starts, but we are doing this already (each worker pings GCS like 5~6 times when a new worker starts). We can fix this holistically when we address large scale cluster problems.

added to comment as TODO

rkooo567 · 2024-02-27T13:52:15Z

python/ray/_private/worker.py

+            return self._cached_driver_node_id
+        all_infos = self.gcs_client.get_all_job_info(timeout=timeout)
+        # Type: src.ray.protobuf.gcs_pb2.JobTableData
+        my_job_info = all_infos[self.current_job_id.binary()]


use type annotation instead?

rkooo567 · 2024-02-27T13:52:37Z

python/ray/_private/worker.py

    # should simply set "global_worker" to equal "None" or something like that.
    global_worker.set_mode(None)
    global_worker.set_cached_job_id(None)
+    global_worker._cached_driver_node_id = None


Can you add type annotation here?

done in init

rkooo567 · 2024-02-27T13:53:23Z

Please make sure to change docstring to the consistent style!

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rkooo567 · 2024-02-27T21:28:09Z

lmk when it is mergeable!

rkooo567 · 2024-02-28T14:52:37Z

premerge failure

…on). Instead, populate it to JobConfig in core worker ctor Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang · 2024-03-01T04:17:36Z

Premerge failure due to it adds a read to gcs on core worker creation. It can happen very slow (as slow as 3 second in bad cases) and makes some unit tests to timeout. Fixed by removing the read, instead, populate driver_node_id in JobConfig in driver's core worker ctor.

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

…ath (#43899) Re: #43214 There is a core change that stopped adding script directory and current directory for different conditions. This accidentally stopped adding current directory when it's coming from the dashboard where in that case we only want to stop adding the script directory. This PR fixes the issue and allow serve deploy to continue working with the current directory. --------- Signed-off-by: Gene Su <e870252314@gmail.com>

…ath (ray-project#43899) Re: ray-project#43214 There is a core change that stopped adding script directory and current directory for different conditions. This accidentally stopped adding current directory when it's coming from the dashboard where in that case we only want to stop adding the script directory. This PR fixes the issue and allow serve deploy to continue working with the current directory. --------- Signed-off-by: Gene Su <e870252314@gmail.com>

…ath (#43899) (#44011) Cherry-pick #43899 Re: #43214 There is a core change that stopped adding script directory and current directory for different conditions. This accidentally stopped adding current directory when it's coming from the dashboard where in that case we only want to stop adding the script directory. This PR fixes the issue and allow serve deploy to continue working with the current directory. --------- Signed-off-by: Gene Su <e870252314@gmail.com>

…ray-project#43214)" This reverts commit b7a6d6d. Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

…#43214)" (#44435) This reverts commit b7a6d6d. Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

On working-dir, don't set _py_driver_sys_path.

7a82393

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang assigned rkooo567 and scv119 Feb 16, 2024

rkooo567 reviewed Feb 20, 2024

View reviewed changes

rynewang added 5 commits February 21, 2024 13:50

Merge branch 'master' into no-syspath-workingdir

41e48a9

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

update comment, fix test

2d77486

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

fix test

3bc01b3

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

moved test to a nicer neighborhood (no module level fixture)

6cea3f7

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

no jobs in minimal

6732522

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang and others added 4 commits February 22, 2024 11:36

Merge branch 'master' into no-syspath-workingdir

33d6c87

fix windows 4

422db70

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

Merge remote-tracking branch 'ryw/no-syspath-workingdir' into no-sysp…

59a5c75

…ath-workingdir Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

serve: add data deps, previously it got an implicit dep that happened…

6f0a355

… to work because we checked the local dir, defying the point of the whole test. Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rkooo567 reviewed Feb 26, 2024

View reviewed changes

rynewang assigned rynewang and unassigned scv119 and rkooo567 Feb 26, 2024

add sys path only on the driver's node, by checking it on job config …

41f53e5

…init. Added tests Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang assigned rkooo567 Feb 27, 2024

rkooo567 approved these changes Feb 27, 2024

View reviewed changes

update doc and type annotation

6bd0458

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rkooo567 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Feb 28, 2024

rynewang and others added 2 commits February 28, 2024 23:29

Merge branch 'master' into no-syspath-workingdir

c5a2fe9

don't read gcs for it - it's so slow (as slow as 3s per worker creati…

59e4fd8

…on). Instead, populate it to JobConfig in core worker ctor Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang requested review from a team, ericl, pcmoritz and raulchen as code owners March 1, 2024 04:15

rynewang and others added 2 commits February 29, 2024 20:35

remove useless import

99675a0

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

Merge branch 'master' into no-syspath-workingdir

0573b75

rkooo567 merged commit b7a6d6d into ray-project:master Mar 1, 2024

rynewang deleted the no-syspath-workingdir branch March 2, 2024 00:39

GeneDer mentioned this pull request Mar 12, 2024

[Serve] Fix serve deploy not getting current directory added to sys.path #43899

Merged

8 tasks

GeneDer mentioned this pull request Mar 14, 2024

[Serve] Fix serve deploy not getting current directory added to sys.path (#43899) #44011

Merged

8 tasks

GeneDer mentioned this pull request Mar 31, 2024

[Core] Allow py_driver_sys_path to be added to sys.path regardless the node #44382

Closed

8 tasks

rynewang added a commit to rynewang/ray that referenced this pull request Apr 2, 2024

Revert "[core] If there's working_dir, don't set _py_driver_sys_path. (…

4229307

…ray-project#43214)" This reverts commit b7a6d6d. Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang mentioned this pull request Apr 2, 2024

Revert "[core] If there's working_dir, don't set _py_driver_sys_path. (#43214) #44435

Merged

jjyao pushed a commit that referenced this pull request Apr 3, 2024

Revert "[core] If there's working_dir, don't set _py_driver_sys_path. (…

1dae8c0

…#43214)" (#44435) This reverts commit b7a6d6d. Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

[core] If there's working_dir, don't set _py_driver_sys_path. #43214

[core] If there's working_dir, don't set _py_driver_sys_path. #43214

Uh oh!

Conversation

rynewang commented Feb 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkooo567 commented Feb 16, 2024

Uh oh!

rkooo567 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkooo567 Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rynewang commented Feb 22, 2024

Uh oh!

rkooo567 left a comment

Choose a reason for hiding this comment

Uh oh!

rynewang commented Feb 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkooo567 commented Feb 27, 2024

Uh oh!

rkooo567 commented Feb 27, 2024

Uh oh!

rkooo567 commented Feb 28, 2024

Uh oh!

rynewang commented Mar 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rynewang commented Feb 16, 2024 •

edited

Loading

rkooo567 Feb 26, 2024 •

edited

Loading