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

Allow for the creation of unique subfolders in the remote_workdir #57

Merged
merged 55 commits into from
May 12, 2023
Merged

Allow for the creation of unique subfolders in the remote_workdir #57

merged 55 commits into from
May 12, 2023

Conversation

Andrew-S-Rosen
Copy link
Contributor

@Andrew-S-Rosen Andrew-S-Rosen commented May 8, 2023

Closes #56 and combines a separate bugfix I was going to introduce in #58.

### Added

- A new kwarg `create_unique_workdir` that will create unique subfolders of the type `<DISPATCH ID>/node_<NODE ID>` within `remote_workdir` if set to `True`


### Fixed

- Fixed a bug where `cleanup = False` would be ignored.
- Fixed a bug where if `cache_dir` was not present, Covalent would crash.

@Andrew-S-Rosen
Copy link
Contributor Author

Andrew-S-Rosen commented May 8, 2023

@wjcunningham7, @santoshkumarradha, @cjao: This should be good to go I think! I just tested it out, and it worked as expected.

Since we will want to mimic this approach in other executors, please make sure the naming and such are to your liking. When it's merged, can we also get a version update? Thanks!

@codecov
Copy link

codecov bot commented May 8, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.36 🎉

Comparison is base (ba35b6c) 84.06% compared to head (cf1da17) 84.43%.

❗ Current head cf1da17 differs from pull request most recent head 81b687a. Consider uploading reports for the commit 81b687a to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop      #57      +/-   ##
===========================================
+ Coverage    84.06%   84.43%   +0.36%     
===========================================
  Files            2        2              
  Lines          295      302       +7     
===========================================
+ Hits           248      255       +7     
  Misses          47       47              
Impacted Files Coverage Δ
covalent_slurm_plugin/slurm.py 84.38% <100.00%> (+0.37%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Andrew-S-Rosen Andrew-S-Rosen changed the title [WIP] Allow for the creation of unique subfolders in the remote_workdir Allow for the creation of unique subfolders in the remote_workdir May 8, 2023
@santoshkumarradha
Copy link
Member

Wow this was super fast and perfect ! 🙏

CC : @wjcunningham7

Copy link
Member

@wjcunningham7 wjcunningham7 left a comment

Choose a reason for hiding this comment

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

Last comment is that the unique directory should be conveyed to Slurm using the --chdir option.

covalent_slurm_plugin/slurm.py Outdated Show resolved Hide resolved
covalent_slurm_plugin/slurm.py Outdated Show resolved Hide resolved
covalent_slurm_plugin/slurm.py Outdated Show resolved Hide resolved
covalent_slurm_plugin/slurm.py Outdated Show resolved Hide resolved
covalent_slurm_plugin/slurm.py Outdated Show resolved Hide resolved
covalent_slurm_plugin/slurm.py Outdated Show resolved Hide resolved
covalent_slurm_plugin/slurm.py Outdated Show resolved Hide resolved
covalent_slurm_plugin/slurm.py Outdated Show resolved Hide resolved
covalent_slurm_plugin/slurm.py Outdated Show resolved Hide resolved
covalent_slurm_plugin/slurm.py Outdated Show resolved Hide resolved
@Andrew-S-Rosen
Copy link
Contributor Author

Andrew-S-Rosen commented May 10, 2023

EDIT: Fixed and ready to go!! I confirmed that everything works correctly in a "real" scenario on Perlmutter.

Alright, I'm a bit stumped. Entirely unrelated to this PR, I'm having that issue where I cannot unpickle things on the remote machine (Perlmutter). I tried nuking my entire conda env again on both local and remote but with no luck. That's making it a bit hard to test this and give me confidence that everything is working perfectly.

I also think I must have introduced an error somewhere when switching from self._current_remote_workdir to passing the currente_remote_workdir around because the unique folders aren't being made anymore when I tried it out "for real" (despite the tests passing here).

Going to have to tackle this tomorrow because I have no brain cells left right now...

for more information, see https://pre-commit.ci

update test


black


fix test


Move cache_dir kwarg
@Andrew-S-Rosen Andrew-S-Rosen changed the title Allow for the creation of unique subfolders in the remote_workdir [WIP] Allow for the creation of unique subfolders in the remote_workdir May 10, 2023
@Andrew-S-Rosen Andrew-S-Rosen changed the title [WIP] Allow for the creation of unique subfolders in the remote_workdir Allow for the creation of unique subfolders in the remote_workdir May 11, 2023
wjcunningham7
wjcunningham7 previously approved these changes May 12, 2023
@wjcunningham7 wjcunningham7 merged commit cd9de38 into AgnostiqHQ:develop May 12, 2023
14 checks passed
@Andrew-S-Rosen Andrew-S-Rosen deleted the unique-folders branch May 12, 2023 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow for the creation of unique subfolders in the current working directory to avoid file overwriting
3 participants