-
Notifications
You must be signed in to change notification settings - Fork 6k
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
[runtime_env] Change default working_dir
behavior to replace local cached version entirely
#49313
[runtime_env] Change default working_dir
behavior to replace local cached version entirely
#49313
Conversation
Signed-off-by: ujjawal-khare <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's a merge conflict, can you update
Signed-off-by: ujjawal-khare <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
LGTM, needs Train approval @justinvyu |
return [ | ||
os.path.relpath(file_info.path.lstrip("/"), start=fs_path.lstrip("/")) | ||
os.path.relpath(os.path.abspath(file_info.path), start=os.path.abspath(fs_path)) | ||
for file_info in fs.get_file_info(selector) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these changes needed for this PR? Converting to abspath before doing relpath is pretty confusing for non-local filesystems (such as S3) where abs path doesn't make sense.
Let's revert this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@justinvyu The test case was failing Buildkite Link due to an issue with path handling. Specifically, the file system mount was unable to locate the directory because of a mismatch between file_info.path and fs_path. To resolve this, I converted the paths to abs_path to ensure consistency and proper resolution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do you see that this test fails due to this line of code? The change above seems unrelated to this code.
As I mentioned above, we should not use abs path for this method anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@justinvyu have reverted the change and tests are passing as well. Can you please check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ujjawal-khare-27 Sorry for missing this -- after the revert my approval is no longer needed. I'll just merge the PR now.
Signed-off-by: ujjawal-khare <[email protected]>
working_dir
behavior to replace local cached version entirely
Summary
The runtime environment working directory is not refreshed upon multiple job submissions to the same Ray cluster due to a local cached version of the working directory existing and not fetching the updated version of the directory on S3. This PR changes the default behavior to delete and fetch from S3 again rather than use the cached local directory fetched on the first job submission. This change only applies to the case where multiple jobs are submitted in a row with the same
working_dir
url.Related issue number
Closes #49036
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.