Skip to content
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

Refactor and streamline copy_files handling #1759

Merged
merged 68 commits into from
Feb 29, 2024
Merged

Refactor and streamline copy_files handling #1759

merged 68 commits into from
Feb 29, 2024

Conversation

Andrew-S-Rosen
Copy link
Member

@Andrew-S-Rosen Andrew-S-Rosen commented Feb 24, 2024

Closes #1742.

Note:

Motivation

In short, the copy_files handling was in desperate need of a behind-the-scenes overhaul. There are several reasons for this.

  1. As noted in Copy file passing breaks in dispatched workflows  #1742, you can't do Path() on a Future, which means any recipes doing Path() (or string concatenation of paths) in a Flow are going to crash when dispatched over a workflow engine.
  2. There are several file handling functions that do redundant things or are handled in existing dependencies: copy_decompress_files, copy_decompress_tree, copy_decompress_files_from_dir.
  3. Because of Point 2, not all the functions handle things in the same way. For instance, only some have glob support right now, which is confusing.
  4. There are many instances where the type hints are simply inaccurate. For instance, in the Espresso recipes, the prev_dir: str | Path positional argument is often getting dict entries thrown into, as a result of pw_copy_files and such (see the test suite).

Because of this mess, things were in desperate need of a revamp, which this PR addresses.

User-Facing Changes

In most cases, there are no breaking changes to users and is mostly an internal refactor. Some positional arguments of prev_dir have been renamed copy_files for consistency with the type hint, although no user changes are expected if used as positional. Some internal functions have slightly modified behavior, such as copy_decompress_files. Refer to the function signature for more.

@Andrew-S-Rosen Andrew-S-Rosen changed the title Improve copy_files handling Improve copy_files handling: Part 1/3 Feb 25, 2024
Andrew-S-Rosen and others added 8 commits February 25, 2024 10:00
This commit fixes the style issues introduced in 02a6a2f according to the output
from Black, isort and Prettier.

Details: #1759
This commit fixes the style issues introduced in d1e59d8 according to the output
from Black, isort and Prettier.

Details: #1759
@Andrew-S-Rosen
Copy link
Member Author

Thanks, @Nekkrad! Much appreciated. I'll likely go ahead and merge this in the evening here so that it can unblock a few other PRs (including one of yours). I'll make any changes to your PR to ensure everything passes after I do --- no need for you to do anything there. In any case, I wanted to communicate this minor but notable change in terms of how files are handled.

@tomdemeyere
Copy link
Contributor

Haven't had time to look at this in details (I am off this week). I think your last sentence summarise very well my first thoughts about this:

Users have full flexibility now, and everything is internally consistent.

Which is very positive

@Andrew-S-Rosen
Copy link
Member Author

Andrew-S-Rosen commented Feb 28, 2024

@tomdemeyere No worries. Try not to open GitHub too much on your time off. 😉

Much of this refactoring was due to nice feature additions you made, so thank you! I will go ahead and merge this shortly, but when you return, don't hesitate to reach out if there are any issues/concerns.

@Andrew-S-Rosen Andrew-S-Rosen added bug Something isn't working enhancement New feature or request housekeeping Things to tidy up that are otherwise kind of boring labels Feb 29, 2024
@Andrew-S-Rosen Andrew-S-Rosen changed the title Refactor, improve, and occasionally modify copy_files handling Refactor and streamline copy_files handling Feb 29, 2024
@Andrew-S-Rosen
Copy link
Member Author

Alright, this sucked up half a week of my life but in the end, there should be no major breaking changes (while also resolving #1742). There are some gnarly Future-resolving things for me to solve with Parsl, but that's for later and shouldn't impact current recipes. Refer to the PR summary for details.

@Andrew-S-Rosen Andrew-S-Rosen merged commit 406c2b7 into main Feb 29, 2024
21 checks passed
@Andrew-S-Rosen Andrew-S-Rosen deleted the paths branch February 29, 2024 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request housekeeping Things to tidy up that are otherwise kind of boring
Development

Successfully merging this pull request may close these issues.

Copy file passing breaks in dispatched workflows
3 participants