[runtime env] Allow __MACOS folder and multiple top-level directories for remote URI zip #29010
Conversation
Signed-off-by: Archit Kulkarni <[email protected]>
Signed-off-by: Archit Kulkarni <[email protected]>
| "directory of example_dir when creating the zip file. " | ||
| "You can check the contents with `zipinfo -1 example.zip`." | ||
| ) | ||
| if top_level_directory is not None: |
There was a problem hiding this comment.
This is always true right? because top_level_directory is a boolean from the args
There was a problem hiding this comment.
You might be thinking of remove_top_level_directory?
There was a problem hiding this comment.
get_top_level_dir_from_compressed_package returns None if there isn't a unique top-level directory (not counting __MACOS
There was a problem hiding this comment.
Nice work, these changes look good. I left a couple comments. Could you update the documentation to remove (e.g. __MACOSX/)?
| ) | ||
| if top_level_directory is not None: | ||
| # Remove __MACOSX directory if it exists | ||
| macos_dir = os.path.join(target_dir, MAC_OS_ZIP_HIDDEN_DIR_NAME) |
There was a problem hiding this comment.
Should we also remove the __MACOSX directory even if there's no top-level directory?
There was a problem hiding this comment.
I don't have a strong preference, but I would prefer not to, just to change as little possible.
There was a problem hiding this comment.
I.e. to only manually delete things if we really have to
Co-authored-by: shrekris-anyscale <[email protected]> Signed-off-by: Archit Kulkarni <[email protected]>
|
Oh great point, I forgot to update the documentation |
Signed-off-by: Archit Kulkarni <[email protected]>
…hitkulkarni/ray into runtime-env-zip-requirements Signed-off-by: Archit Kulkarni <[email protected]>
|
@shrekris-anyscale, I updated the docs. Do you think we should actually remove the entire part in the docs about requiring a top-level directory, or leave it the way it is? It depends whether we want to treat the multiple top-level directory as a recommended path, or treat it as an edge case / user error that we're handling gracefully. |
|
cc @pcmoritz for docs codeowner review, and in case you want to weigh in on the docs question above |
| You can avoid this by using the ``zip -r`` command directly on the directory you want to compress. Make sure to run the command from that directory's parent. | ||
| If Ray detects more than a single directory at the top level, it will use the entire zip file instead of the top-level directory, which may lead to unexpected behavior. |
There was a problem hiding this comment.
To avoid this, use the zip -r command directly on the directory you want to compress from its parent's directory. For example, if you have a directory structure such as: a/b and you what to compress b, issue the zip -r b from the directory a.
There was a problem hiding this comment.
I assume that's what you mean with zip -r?
There was a problem hiding this comment.
Yes, that's correct and the way you've stated it is cleaner. I'll update this.
Signed-off-by: Archit Kulkarni <[email protected]>
…ime-env-zip-requirements Too many flaky tests.
|
Of the tests which failed in both this commit (merge master) and the previous commit: |
… for remote URI zip (ray-project#29010) Implements the changes proposed in ray-project#28859, please see the issue for full details. Copied from the issue: If there is a single top level directory and possibly a __MACOS folder, stick to the current behavior and use the top level directory in the zip file as the working directory (this is the current behavior, but also fixes the __MACOS folder problem) If there are multiple files or folders in the .zip file, take the contents of the zip file itself as the working directory. This behavior will be fully backwards compatible and supports a common format of .zip files (i.e. put all the files into a top level directory), yet it also allows users to put files directly into the .zip file and removes the footgun. Signed-off-by: Weichen Xu <[email protected]>
Signed-off-by: Archit Kulkarni [email protected]
Why are these changes needed?
Implements the changes proposed in #28859, please see the issue for full details.
Copied from the issue:
Related issue number
Closes #28859
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.