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

Get rid of redundant function calls on Ray workers #3599

Closed
YarShev opened this issue Oct 26, 2021 · 8 comments
Closed

Get rid of redundant function calls on Ray workers #3599

YarShev opened this issue Oct 26, 2021 · 8 comments
Labels
Ray ⚡ Issues related to the Ray engine

Comments

@YarShev
Copy link
Collaborator

YarShev commented Oct 26, 2021

We have had a hacky solution to prevent #3014, #647, #746. We should try to get rid of it since we use Ray functionality that is not directly supposed to be used.

@YarShev YarShev changed the title Get rid of function calls on Ray workers Get rid of redundant function calls on Ray workers Oct 26, 2021
YarShev added a commit to YarShev/modin that referenced this issue Oct 26, 2021
…rkers

Signed-off-by: Igoshev, Yaroslav <yaroslav.igoshev@intel.com>
@vnlitvinov
Copy link
Collaborator

Is the issue we're working around really gone?.. If it isn't this is going to hamper the stability a lot (and my impression is the issue in Ray is still there).

@YarShev
Copy link
Collaborator Author

YarShev commented Oct 26, 2021

I do not see the issue locally and there is no issue in our CI. Also, there is an issue #3601 where a run of pandas imports on Ray workers seems to be a problem.

@devin-petersohn
Copy link
Collaborator

It's most evident on large machines with 100+ CPU cores. I don't trust that the bug is gone in Ray as long as the import thread is separate from the worker thread. I prefer to leave the code as it is.

Is there a problem with using these functionalities in Ray?

@YarShev
Copy link
Collaborator Author

YarShev commented Oct 27, 2021

I had run Modin tests on a machine with 112 CPU cores and didn't see any fail. Let's wait @rkooo567's answer in #3601 on the status of the problem.

Is there a problem with using these functionalities in Ray?

I prefer Modin (and probably other users/packages) used high-level API (link) from Ray and other packages and didn't not rely on their internals.

@devin-petersohn
Copy link
Collaborator

I am strongly against removing it. I spent probably 100 hours debugging and trying to get to a solution for the import thread race condition, even when engaging with the Ray team. Yes, it's not ideal to use internal APIs, but it is too important and I have spent too much time on it. It's not something that's even easily reproduced, and as long as Ray chooses to import in a separate thread from running actual code, I do not trust that design for Modin workloads.

@YarShev
Copy link
Collaborator Author

YarShev commented Oct 28, 2021

Makes sense. Let's follow the discussion in #3601 for now.

@anmyachev anmyachev added the Ray ⚡ Issues related to the Ray engine label Apr 21, 2022
@RehanSD
Copy link
Collaborator

RehanSD commented Oct 12, 2022

Hi @YarShev @vnlitvinov @devin-petersohn has this issue been resolved?

@mvashishtha
Copy link
Collaborator

@RehanSD I think #4603 fixed this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ray ⚡ Issues related to the Ray engine
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants