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

Remove post-execute hook from ray task #2305

Merged
merged 1 commit into from
Mar 29, 2024
Merged

Conversation

yundai424
Copy link
Contributor

@yundai424 yundai424 commented Mar 28, 2024

Issue: ray-project/kuberay#1975

Why are the changes needed?

ray.shutdown() will always be automatically called (registered through atexit here at ray import time). We should rather rely on the one registered with atexit hook instead of explicitly calling it again especially as we don't set _exiting_interpreter=True which may result in disconnecting too soon before worker logs are flushed to driver.

What changes were proposed in this pull request?

remove the post_execute of ray task

How was this patch tested?

test with a simple example:

import typing

from flytekit import ImageSpec, Resources, task, workflow

commit_hash = "03fd5605996e53e848ba77e48608e3395189319a"
ray_plugin = f"git+https://github.com/yundai424/flytekit.git@{commit_hash}#subdirectory=plugins/flytekit-ray"

custom_image = ImageSpec(
    name="ray-flyte-plugin",
    registry="localhost:30000",
    packages=[ray_plugin],
    apt_packages=['git', 'wget', 'vim'],
)

# if custom_image.is_container():
import ray
from flytekitplugins.ray import HeadNodeConfig, RayJobConfig, WorkerNodeConfig

@ray.remote
def f(x):
    print(f"ray task get {x}")
    return x * x

@task(
    task_config=RayJobConfig(
        head_node_config=HeadNodeConfig(ray_start_params={"log-color": "True", "block": "true"}),
        worker_node_config=[WorkerNodeConfig(group_name="ray-group", replicas=1)],
        runtime_env={"pip": []},
    ),
    requests=Resources(mem="4Gi", cpu="1"),
    container_image=custom_image,
)
def ray_task(n: int) -> typing.List[int]:
    futures = [f.remote(i) for i in range(n)]
    l = ray.get(futures)
    print(l, 2)
    return l

@workflow
def ray_workflow(n: int) -> typing.List[int]:
    return ray_task(n=n)

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Mar 28, 2024
@yundai424 yundai424 changed the title Remove post-execute hook for ray task Remove post-execute hook from ray task Mar 28, 2024
@dosubot dosubot bot added the lgtm This PR has been approved by maintainer label Mar 28, 2024
Copy link

codecov bot commented Mar 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.48%. Comparing base (8ab9a3c) to head (03fd560).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2305   +/-   ##
=======================================
  Coverage   83.48%   83.48%           
=======================================
  Files         324      324           
  Lines       24720    24720           
  Branches     3517     3517           
=======================================
+ Hits        20637    20638    +1     
+ Misses       3451     3450    -1     
  Partials      632      632           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pingsutw pingsutw merged commit 9aaf160 into flyteorg:master Mar 29, 2024
48 checks passed
Copy link

welcome bot commented Mar 29, 2024

Congrats on merging your first pull request! 🎉

fiedlerNr9 pushed a commit that referenced this pull request Jul 25, 2024
Signed-off-by: Yun Dai <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by maintainer size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants