Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add as_posix to s3 push file paths in deployment step #314

Merged
merged 3 commits into from
Oct 3, 2023

Conversation

markbruning
Copy link
Contributor

Add as_posix test

Completed tests

Closes #309

Example

Now handles windows pathing appropriately as posix paths when writing to S3

Screenshots

N/A, only affects existing processing

Checklist

  • References any related issue by including "Closes #" or "Closes ".
    • If no issue exists and your change is not a small fix, please create an issue first.
  • Includes tests or only affects documentation.
  • Passes pre-commit checks.
    • Run pre-commit install && pre-commit run --all locally for formatting and linting.
  • Includes screenshots of documentation updates.
    • Run mkdocs serve view documentation locally.
  • Summarizes PR's changes in CHANGELOG.md

@markbruning markbruning requested a review from a team as a code owner September 18, 2023 16:40
@markbruning
Copy link
Contributor Author

@desertaxle you were an active maintainer, so I'm pinging you to see if I can get this PR looked at?

Copy link
Member

@desertaxle desertaxle left a comment

Choose a reason for hiding this comment

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

It works for Windows now but is failing when run on Linux. I made a suggestion that should fix the behavior on both platforms.

@@ -117,7 +117,9 @@ def push_to_s3(
continue
elif not local_file_path.is_dir():
remote_file_path = Path(folder) / local_file_path.relative_to(local_path)
client.upload_file(str(local_file_path), bucket, str(remote_file_path))
client.upload_file(
str(local_file_path), bucket, str(remote_file_path.as_posix())
Copy link
Member

Choose a reason for hiding this comment

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

This trick can help ensure it works on Windows and Linux platforms.

Suggested change
str(local_file_path), bucket, str(remote_file_path.as_posix())
str(local_file_path), bucket, str(PureWindowsPath(remote_file_path.as_posix()))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@desertaxle I think it's a problem in the test, not in the code.

I ran the following code on both windows and linux:

from pathlib import Path
folder = 'arbitrary/s3/folder/'
local_path = Path.cwd()
for local_file_path in local_path.expanduser().rglob("*"):
    if not local_file_path.is_dir():
        remote_file_path = Path(folder) / local_file_path.relative_to(local_path)
        print(f"not_posix: {str(remote_file_path)}")
        print(f"yes_posix: {str(remote_file_path.as_posix())}")

Windows yielded:

not_posix: arbitrary\s3\folder\top.txt
yes_posix: arbitrary/s3/folder/top.txt
not_posix: arbitrary\s3\folder\dir1\sub.txt
yes_posix: arbitrary/s3/folder/dir1/sub.txt

Linux yielded:

not_posix: arbitrary/s3/folder/top.txt
yes_posix: arbitrary/s3/folder/top.txt
not_posix: arbitrary/s3/folder/dir1/sub.txt
yes_posix: arbitrary/s3/folder/dir1/sub.txt

So I think that the code that I wrote will not break based on OS, but I might need to make the mock of the returned files more accurate. Thoughts? Am I missing something?

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 the issue might be in the tmp_file_win fixture. On Linux it's likely escaping the \ to create a valid posix path. I'd recommend updating test_push_tos3_as_posix to only run on Windows. You can do that like this: @pytest.mark.skipif(sys.platform != 'win32', reason="requires Windows")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@desertaxle done and done

@markbruning
Copy link
Contributor Author

@desertaxle anything left before this can be merged?

Copy link
Member

@desertaxle desertaxle left a comment

Choose a reason for hiding this comment

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

LGTM!

@desertaxle desertaxle merged commit 50ad8f8 into PrefectHQ:main Oct 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

push_to_s3 deployment step from windows machine should use posix separator when uploading folders
2 participants