FIX-#4479: Prevent users from using a local filepath when performing a distributed write#4484
Open
RehanSD wants to merge 16 commits intomodin-project:masterfrom
Open
FIX-#4479: Prevent users from using a local filepath when performing a distributed write#4484RehanSD wants to merge 16 commits intomodin-project:masterfrom
RehanSD wants to merge 16 commits intomodin-project:masterfrom
Conversation
…n performing a distributed write Signed-off-by: Rehan Durrani <rehan@ponder.io>
added 2 commits
May 23, 2022 12:32
Signed-off-by: Rehan Durrani <rehan@ponder.io>
|
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
Collaborator
mvashishtha
left a comment
mvashishtha
left a comment
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
Collaborator
naren-ponder
left a comment
naren-ponder
left a comment
There was a problem hiding this comment.
Does this have to be added for to_sql as well?
| if not cls._to_csv_check_support(kwargs): | ||
| return RayIO.to_csv(qc, **kwargs) | ||
|
|
||
| if len(ray.nodes()) > 1: |
Collaborator
There was a problem hiding this comment.
Can this be moved to _to_csv_check_support, ditto for parquet?
Collaborator
Author
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) |
Collaborator
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.
Collaborator
Author
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 <Poolliver868@mail.ru>
Co-authored-by: Mahesh Vashishtha <mvashishtha@users.noreply.github.com>
Co-authored-by: Mahesh Vashishtha <mvashishtha@users.noreply.github.com>
Co-authored-by: Mahesh Vashishtha <mvashishtha@users.noreply.github.com>
Collaborator
Author
|
We don't need to do this for |
added 2 commits
May 25, 2022 20:35
Signed-off-by: Rehan Durrani <rehan@ponder.io>
…tent local paths Signed-off-by: Rehan Durrani <rehan@ponder.io>
|
This pull request introduces 1 alert when merging dcadcf4 into 60518ca - view on LGTM.com new alerts:
|
mvashishtha
reviewed
May 26, 2022
Collaborator
mvashishtha
left a comment
mvashishtha
left a comment
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 <mvashishtha@users.noreply.github.com>
Co-authored-by: Mahesh Vashishtha <mvashishtha@users.noreply.github.com>
Signed-off-by: Rehan Durrani <rehan@ponder.io>
|
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
added 4 commits
June 1, 2022 17:31
Signed-off-by: Rehan Durrani <rehan@ponder.io>
Signed-off-by: Rehan Durrani <rehan@ponder.io>
Signed-off-by: Rehan Durrani <rehan@ponder.io>
Signed-off-by: Rehan Durrani <rehan@ponder.io>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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 rehan@ponder.io
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