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

Add prepend_script and append_script for job.resource #273

Merged
merged 10 commits into from Nov 16, 2022

Conversation

Cloudac7
Copy link
Contributor

@Cloudac7 Cloudac7 commented Nov 15, 2022

It is sometimes in need of executing command lines before or after task submitted to startup or shutdown necessary environment. While most issue with prepend script required like this might be solved by source a static script on remote server (which might also be difficult when remote server is not so stable.), scripts to be executed after tasks finished might be difficult under current dpdispatcher resource.
Here in this PR, prepend_script and append_script parameters have been add to job.resource in form of List, in which a single line could be an item, like this:

"prepend_script": [
    "conda activate test_env",
    "export PATH=/path/to/package:$PATH",
    "send_an_email_to 114@514.com"
    "sleep 1919810"
]

and the expected output:

conda activate test_env
export PATH=/path/to/package:$PATH
send_an_email_to 114@514.com
sleep 1919810

Another little change is just add delay=True for logger to prevent the generation of dpdispatcher.log even if no log information output.

@Cloudac7
Copy link
Contributor Author

Cloudac7 commented Nov 15, 2022

For prepend_script, it could also be an optional choice for now existing module_list, source_list, envs, etc. But the latter ones could not cover all requirement for executing a script before tasks submitted. Also procedure of tasks could be customized by user instead of the predefined module -> source -> env one.

dpdispatcher/distributed_shell.py Outdated Show resolved Hide resolved
dpdispatcher/distributed_shell.py Outdated Show resolved Hide resolved
dpdispatcher/machine.py Outdated Show resolved Hide resolved
dpdispatcher/machine.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Nov 16, 2022

Codecov Report

Base: 58.63% // Head: 59.44% // Increases project coverage by +0.81% 🎉

Coverage data is based on head (390ff61) compared to base (eb86fc0).
Patch coverage: 71.42% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #273      +/-   ##
==========================================
+ Coverage   58.63%   59.44%   +0.81%     
==========================================
  Files          26       27       +1     
  Lines        2879     2895      +16     
==========================================
+ Hits         1688     1721      +33     
+ Misses       1191     1174      -17     
Impacted Files Coverage Δ
dpdispatcher/distributed_shell.py 17.64% <0.00%> (-0.88%) ⬇️
dpdispatcher/lsf.py 37.36% <ø> (+14.28%) ⬆️
dpdispatcher/machine.py 86.27% <100.00%> (+4.27%) ⬆️
dpdispatcher/submission.py 84.33% <100.00%> (+0.21%) ⬆️
dpdispatcher/_version.py 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

* add script rendering test for each part
@Cloudac7
Copy link
Contributor Author

Cloudac7 commented Nov 16, 2022

Well, it might be my ignorance that not using string.join() for I just thought error was raised when empty list passed, while it won't. It also hint me that the unittests not include a tempelate rendering test, it might be a good idea to add one.

Well, local test for script generation was add for Slurm, but not including env and end. Here considering coverage, I just add one for LSF and thus test for other parts in script in a local way.

@Cloudac7 Cloudac7 requested a review from njzjz November 16, 2022 02:56
dpdispatcher/distributed_shell.py Outdated Show resolved Hide resolved
dpdispatcher/distributed_shell.py Outdated Show resolved Hide resolved
dpdispatcher/machine.py Outdated Show resolved Hide resolved
dpdispatcher/machine.py Outdated Show resolved Hide resolved
@Cloudac7
Copy link
Contributor Author

This line is useless.

Fixed.

@Cloudac7 Cloudac7 requested a review from njzjz November 16, 2022 03:12
@njzjz njzjz merged commit e59e73a into deepmodeling:master Nov 16, 2022
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.

None yet

3 participants