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

read_excel leaves ray logs open #3818

Closed
jeffreykennethli opened this issue Dec 8, 2021 · 5 comments · Fixed by #3826
Closed

read_excel leaves ray logs open #3818

jeffreykennethli opened this issue Dec 8, 2021 · 5 comments · Fixed by #3826
Assignees
Labels
bug 🦗 Something isn't working Ray ⚡ Issues related to the Ray engine Testing 📈 Issues related to testing

Comments

@jeffreykennethli
Copy link

Describe the problem

After running read_excel, there are open file handles for ray session logs:

in ipython:
Screen Shot 2021-12-08 at 12 51 56 PM

in test_io.py:
Screen Shot 2021-12-08 at 2 46 08 PM

Interestingly for test_io.py::TestExcel, only one of the test cases fails with this assertion if you run them together, but if you run them individually, they all fail.

Is related to #2559 but the root cause is probably different. To check open files, use

import psutil
p = psutil.Process()
# run code
print(p.open_files())
@jeffreykennethli jeffreykennethli self-assigned this Dec 8, 2021
@jeffreykennethli jeffreykennethli added bug 🦗 Something isn't working Ray ⚡ Issues related to the Ray engine Testing 📈 Issues related to testing labels Dec 8, 2021
@devin-petersohn
Copy link
Collaborator

@modin-project/modin-ray Do we need to manually close the remaining open file handle(s) for ray logs?

@rkooo567
Copy link
Collaborator

rkooo567 commented Dec 8, 2021

Files are open because Ray keeps reading them to stream logs to the driver I think. You can probably do ray.init(log_to_driver=False), then it is not open anymore?

@jeffreykennethli
Copy link
Author

Tried log_to_driver=False, initializing then shutting down ray (ray should be shutdown when the process that uses it (read_excel) doesn't need it, and the open file still appears there:

Screen Shot 2021-12-08 at 6 23 50 PM

More generally, is it intended for the stream logs to still be open after some work is done? If so, maybe the solution on our end is to ignore open driver logs. If not, then @rkooo567 do you have any other ideas how to make sure the stream logs are closed?

@rkooo567
Copy link
Collaborator

rkooo567 commented Dec 9, 2021

Yes it is intended to keep opening files after some work is done. https://github.com/ray-project/ray/blob/5298a9046c68c8577ee5b01371c7aa0a95a915e3/python/ray/_private/log_monitor.py#L235

Seems like files are only closed when there are too many open files (threshold is 200 by default). Otherwise, we keep them open so that we can keep reading new log lines from the file to stream logs to drivers.

However, I think files keep being open after ray.shutdown could be a bug. Files being open with log_to_driver=False seems a bit weird to me. I suspect the log monitor component doesn't have a signal handler to close all opened files.

For this one, can you create an issue to Ray Github?

@jeffreykennethli
Copy link
Author

Thank you! I will refactor the tests to ignore the ray driver logs then.

Opened issue ray-project/ray#20991.

jeffreykennethli pushed a commit to jeffreykennethli/modin that referenced this issue Dec 9, 2021
…cting file leaks

Signed-off-by: jeffreykennethli <jkli@ponder.io>
devin-petersohn pushed a commit that referenced this issue Dec 13, 2021
…aks (#3826)

Signed-off-by: jeffreykennethli <jkli@ponder.io>
dorisjlee pushed a commit to dorisjlee/modin that referenced this issue Jan 13, 2022
…cting file leaks (modin-project#3826)

Signed-off-by: jeffreykennethli <jkli@ponder.io>
Signed-off-by: Doris Lee <dorisjunglinlee@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🦗 Something isn't working Ray ⚡ Issues related to the Ray engine Testing 📈 Issues related to testing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants