Skip to content

[core] Fix test_object_spilling.py on Windows #53851

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

Merged
merged 2 commits into from
Jun 16, 2025

Conversation

edoakes
Copy link
Collaborator

@edoakes edoakes commented Jun 16, 2025

I made the workload more stressful in #53803 by fetching all of the results concurrently. That seems to have caused Windows to time out: https://buildkite.com/ray-project/postmerge/builds/10876#01977755-755b-485e-bd13-f0ea3e33cc36/158-818

I won't pretend to fully understand why, but reverting to the old pattern in an attempt to fix it.

Also added an explicit wait for the dir to drain because there was an error during cleanup caused by it.

edoakes added 2 commits June 16, 2025 09:02
Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
@edoakes edoakes added the go add ONLY when ready to merge, run all tests label Jun 16, 2025
@edoakes edoakes requested a review from a team June 16, 2025 16:15
@edoakes edoakes enabled auto-merge (squash) June 16, 2025 16:15
Comment on lines +755 to +756
for obj_ref in [f.remote() for _ in range(5)]:
ray.get(obj_ref)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is bizarre. I would expect

ray.get([f.remote() for _ in range(5)])

to have better performance since it's fetching a batch instead of blocking on each individual call.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in this case I think it's more stressful because not all 5 objs fit in the object store

@edoakes edoakes merged commit 7766b52 into ray-project:master Jun 16, 2025
5 of 6 checks passed
elliot-barn pushed a commit that referenced this pull request Jun 18, 2025
I made the workload more stressful in
#53803 by fetching all of the
results concurrently. That seems to have caused Windows to time out:
https://buildkite.com/ray-project/postmerge/builds/10876#01977755-755b-485e-bd13-f0ea3e33cc36/158-818

I won't pretend to fully understand why, but reverting to the old
pattern in an attempt to fix it.

Also added an explicit wait for the dir to drain because there was an
error during cleanup caused by it.

---------

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
minerharry pushed a commit to minerharry/ray that referenced this pull request Jun 27, 2025
I made the workload more stressful in
ray-project#53803 by fetching all of the
results concurrently. That seems to have caused Windows to time out:
https://buildkite.com/ray-project/postmerge/builds/10876#01977755-755b-485e-bd13-f0ea3e33cc36/158-818

I won't pretend to fully understand why, but reverting to the old
pattern in an attempt to fix it.

Also added an explicit wait for the dir to drain because there was an
error during cleanup caused by it.

---------

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
elliot-barn pushed a commit that referenced this pull request Jul 2, 2025
I made the workload more stressful in
#53803 by fetching all of the
results concurrently. That seems to have caused Windows to time out:
https://buildkite.com/ray-project/postmerge/builds/10876#01977755-755b-485e-bd13-f0ea3e33cc36/158-818

I won't pretend to fully understand why, but reverting to the old
pattern in an attempt to fix it.

Also added an explicit wait for the dir to drain because there was an
error during cleanup caused by it.

---------

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants