FIX-#4479: Prevent users from using a local filepath when performing a distributed write #4484
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…n performing a distributed write Signed-off-by: Rehan Durrani <[email protected]>
Signed-off-by: Rehan Durrani <[email protected]>
Signed-off-by: Rehan Durrani <[email protected]>
|
Codecov Report
@@ Coverage Diff @@
## master #4484 +/- ##
==========================================
- Coverage 86.82% 78.21% -8.62%
==========================================
Files 222 230 +8
Lines 18038 23692 +5654
==========================================
+ Hits 15662 18530 +2868
- Misses 2376 5162 +2786
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
|
|
YarShev
reviewed
May 24, 2022
mvashishtha
reviewed
May 24, 2022
There was a problem hiding this comment.
@RehanSD thanks for the change. I have some style suggestions.
modin/core/execution/ray/implementations/pandas_on_ray/io/io.py
Outdated
Show resolved
Hide resolved
modin/core/execution/ray/implementations/pandas_on_ray/io/io.py
Outdated
Show resolved
Hide resolved
| @@ -165,6 +166,13 @@ def to_csv(cls, qc, **kwargs): | |||
| if not cls._to_csv_check_support(kwargs): | |||
| return RayIO.to_csv(qc, **kwargs) | |||
|
|
|||
| if len(ray.nodes()) > 1: | |||
There was a problem hiding this comment.
Can this be moved to _to_csv_check_support, ditto for parquet?
There was a problem hiding this comment.
We could, but _to_csv_check_support currently just checks if we can do a parallel write, so it would be a bit of a paradigm shift to raise an error here, unless you're suggesting we return false and default to a serial write on the head node if a local path is passed?
mvashishtha
reviewed
May 24, 2022
| if S3_ADDRESS_REGEX.match(path_or_buf) is not None or "://" in path_or_buf: | ||
| return False # S3 or network path. | ||
| if isinstance(path_or_buf, str) or isinstance(path_or_buf, pathlib.PurePath): | ||
| return os.path.exists(path_or_buf) |
There was a problem hiding this comment.
The code as is will not warn users for the common case where they try to write to a non-existent path, i.e. when they want to create and write to a new file. We should warn even if the path doesn't exist locally.
There was a problem hiding this comment.
Changed it to check device_id instead! I originally did this because apparently os.path.exists returns false for networked file paths. I believe it should now work for both non-existing paths and networked paths.
Co-authored-by: Yaroslav Igoshev <[email protected]>
Co-authored-by: Mahesh Vashishtha <[email protected]>
Co-authored-by: Mahesh Vashishtha <[email protected]>
Co-authored-by: Mahesh Vashishtha <[email protected]>
|
We don't need to do this for |
Signed-off-by: Rehan Durrani <[email protected]>
…tent local paths Signed-off-by: Rehan Durrani <[email protected]>
|
This pull request introduces 1 alert when merging dcadcf4 into 60518ca - view on LGTM.com new alerts:
|
mvashishtha
reviewed
May 26, 2022
There was a problem hiding this comment.
@RehanSD thanks for the changes. I have two minor suggestions. Do we have ray cluster unit tests to which we can add a case for this? Otherwise, have you tested this manually?
Co-authored-by: Mahesh Vashishtha <[email protected]>
Co-authored-by: Mahesh Vashishtha <[email protected]>
Signed-off-by: Rehan Durrani <[email protected]>
|
This pull request introduces 1 alert when merging 0608f9e into 958c26a - view on LGTM.com new alerts:
|
modin/core/execution/ray/implementations/pandas_on_ray/io/io.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Rehan Durrani <[email protected]>
Signed-off-by: Rehan Durrani <[email protected]>
Signed-off-by: Rehan Durrani <[email protected]>
Signed-off-by: Rehan Durrani <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Rehan Durrani [email protected]
What do these changes do?
Prevent users from specifying a local file path on a distributed write.
flake8 modin/ asv_bench/benchmarks scripts/doc_checker.pyblack --check modin/ asv_bench/benchmarks scripts/doc_checker.pygit commit -sdocs/development/architecture.rstis up-to-date