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 pre/post script support #788

Merged
merged 3 commits into from
Mar 26, 2024
Merged

Add pre/post script support #788

merged 3 commits into from
Mar 26, 2024

Conversation

rstyd
Copy link
Collaborator

@rstyd rstyd commented Feb 26, 2024

This PR addresses #773. It adds pre/post script support through a beeflow:ScriptRequirement hint which has the format:

    beeflow:ScriptRequirement:
    enabled: true
    pre_script_path: "pre_run.sh"
    post_script_path: "post_run.sh

The pre_run and post_run scripts are read using the parser and passed to the task object for the associated task. It's then read and added to the SBATCH script in the slurm runner.

@rstyd rstyd requested review from pagrubel, aquan9 and jtronge February 26, 2024 23:30
@pagrubel
Copy link
Collaborator

@rstyd Your example works. Please add it to all the workers and can you set up a test in CI? I think there should be a way to att it to the Flux and Slurm tests using a simpler example.

Copy link
Collaborator

@pagrubel pagrubel left a comment

Choose a reason for hiding this comment

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

As I commented it should be added to all the workers and if possible make CI tests.

@pagrubel
Copy link
Collaborator

  1. Add pre/post to all workers.
  2. Add test to slurm and flux integration test.
  3. Documentation.

@pagrubel pagrubel requested review from pagrubel and kchilleri and removed request for aquan9 and jtronge March 19, 2024 22:31
kchilleri
kchilleri previously approved these changes Mar 20, 2024
Copy link
Collaborator

@kchilleri kchilleri left a comment

Choose a reason for hiding this comment

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

Same pre/post script support will be added to the flux_worker (as requested by Pat) in a separate issue/PR.
Test/documentation TBD?

@pagrubel
Copy link
Collaborator

@kchilleri There may be some misunderstanding. Yes the flux implementation should be separate. However, this PR is still missing documentation and integration tests. So really it shouldn't be approved as is.

@kchilleri
Copy link
Collaborator

@pagrubel Got it! Should I dismiss my approval?

kchilleri
kchilleri previously approved these changes Mar 25, 2024
Copy link
Collaborator

@pagrubel pagrubel left a comment

Choose a reason for hiding this comment

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

Thanks, @rstyd All looks good. Please squash the commits and let me know when you take out your debug Asserts.

docs/sphinx/bee_cwl.rst Outdated Show resolved Hide resolved
@pagrubel
Copy link
Collaborator

@rstyd Please squash the commits, or I can if you wish.

@rstyd rstyd force-pushed the pre_post_script branch from 4066f84 to 15fe121 Compare March 26, 2024 22:54
@pagrubel pagrubel merged commit 8966c51 into develop Mar 26, 2024
4 checks passed
@pagrubel pagrubel deleted the pre_post_script branch March 26, 2024 23:04
@pagrubel pagrubel mentioned this pull request Mar 28, 2024
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.

3 participants