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

fix metadata storage path for RayTaskRunner #16022

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Conversation

zzstoatzz
Copy link
Collaborator

@zzstoatzz zzstoatzz commented Nov 14, 2024

closes #16009

as described in the linked issue, we were writing result metadata files to the cwd when using a ray task runner because child processes would get a None storage basepath - this PR checks for this case and makes sure we also respect the result_storage_key

@github-actions github-actions bot added the bug Something isn't working label Nov 14, 2024
Copy link

codspeed-hq bot commented Nov 14, 2024

CodSpeed Performance Report

Merging #16022 will not alter performance

Comparing fix-ray-results (e07d6c2) with main (be8fa7b)

Summary

✅ 3 untouched benchmarks

@zzstoatzz zzstoatzz force-pushed the fix-ray-results branch 4 times, most recently from ecec785 to 7a41130 Compare November 15, 2024 17:20
revert unintentional log removal

yuhhhhhhhh
dont remove docstring
just install in the action

whoops
@zzstoatzz zzstoatzz self-assigned this Nov 15, 2024
@zzstoatzz zzstoatzz marked this pull request as ready for review November 15, 2024 19:26
if (
self.result_storage is None
and update.get("result_storage") is None
or (self.result_storage and self.result_storage.basepath is None)
Copy link
Member

Choose a reason for hiding this comment

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

I think this change may have unintended consequences since it will now ignore all result storage that doesn't have a basepath, which is valid in cases like storing at the root of an S3 bucket.

I think we may need to dig deeper to find the root cause of this issue and why the block configuration isn't coming through correctly.

@zzstoatzz zzstoatzz marked this pull request as draft November 15, 2024 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metadata files generated with RayTaskRunner
2 participants