Skip to content

Fix script area issue#27

Open
monicagiraldochica wants to merge 22 commits intoRIKEN-RCCS:mainfrom
monicagiraldochica:fix_scriptAreaIssueP
Open

Fix script area issue#27
monicagiraldochica wants to merge 22 commits intoRIKEN-RCCS:mainfrom
monicagiraldochica:fix_scriptAreaIssueP

Conversation

@monicagiraldochica
Copy link
Contributor

Sorry, i deleted the branch by mistake. This is the same pull request as previously.

The app was re-writing any command that the user might type under the SBATCH (or other) heather. I fixed it so that when the script area is re-written, it only re-writes the heather (SBATCH lines) and not any additional commands.

@mnakao
Copy link
Contributor

mnakao commented Jan 11, 2026

Thank you very much for the pull request.
I tested the change and found that it does not work correctly when the script section contains lines other than #! and #SBATCH.
For example, if the script section in sample_apps/Slurm/form.yml is defined as follows, the final mpirun line is not visible:

script: |
  #!/bin/bash
  #SBATCH -p #{partition}
  #SBATCH -n #{cores_memory_1}
  #SBATCH --mem #{cores_memory_2}G
  #SBATCH -t #{time_1}:#{time_2}:00
  #SBATCH --array=#{array_1}-#{array_2}
  #SBATCH --mail-user=#{email}
  #SBATCH --mail-type=#{mail_option}

  mpirun -np 1 ./a.out

In addition, Open Composer generates lines that do not start with #! or #SBATCH as well, but the current implementation does not seem to handle those cases.

@monicagiraldochica
Copy link
Contributor Author

Thank you very much for the pull request. I tested the change and found that it does not work correctly when the script section contains lines other than #! and #SBATCH. For example, if the script section in sample_apps/Slurm/form.yml is defined as follows, the final mpirun line is not visible:

script: |
  #!/bin/bash
  #SBATCH -p #{partition}
  #SBATCH -n #{cores_memory_1}
  #SBATCH --mem #{cores_memory_2}G
  #SBATCH -t #{time_1}:#{time_2}:00
  #SBATCH --array=#{array_1}-#{array_2}
  #SBATCH --mail-user=#{email}
  #SBATCH --mail-type=#{mail_option}

  mpirun -np 1 ./a.out

In addition, Open Composer generates lines that do not start with #! or #SBATCH as well, but the current implementation does not seem to handle those cases.

Hi, thank you for your quick reply! I believe this is fixable. At MCW we only insert #SBATCH lines as part of the heather. So, it was my mistake that I didn't think about all other cases! Oops! But I'll think about other cases and update my pull request, unless I see that you publish an update before with that issue resolved. If it helps, that should require an edit in the helper function that I added. If you're interested, you could let me know all the possible cases in which a line is part of the heather (which are the lines that should be replaced on every update). For example: starts with #SBATCH, starts with #!. And I can edit the helper function. Unless you guys know already how to do the fix or have a more efficient way to do it. I'll give it some thought!

@mnakao
Copy link
Contributor

mnakao commented Jan 13, 2026

Hi, thank you very much for your reply and for your willingness to improve the implementation - we really appreciate your help.

Could you clarify what you mean by “heather”? Do you mean the script header? I just want to make sure I understand it correctly.

After receiving your PR, I also spent some time thinking about the implementation on my side, but unfortunately I couldn’t come up with a clean or clearly better solution either. So your input and ideas are very helpful.

At the very least, all of the samples shown in the following documentation need to work correctly with the final implementation: https://riken-rccs.github.io/OpenComposer/docs/application.html#sample

If you think updating the helper function you added is the right direction, I’m happy to discuss the possible cases together. Thanks again for your cooperation, and I look forward to your thoughts or an updated PR.

@monicagiraldochica
Copy link
Contributor Author

Hi, those issues should be resolved now. I tested that it works on a SLURM scheduler. It should work for others, but i couldnt test because we only have SLURM.

@mnakao
Copy link
Contributor

mnakao commented Jan 28, 2026

Thank you very much for the PR.
However, at least the examples in 2.5. multi_path and 2.8. path do not work correctly.
https://riken-rccs.github.io/OpenComposer/docs/application.html

For example, with the following configuration in 2.5. multi_path:

form:
  load_modules:
    widget: multi_select
    label: Add modules
    value: mpi/mpich-x86_64
    options:
      - [ mpi/mpich-x86_64, mpi/mpich-x86_64 ]
      - [ mpi/openmpi-x86_64, mpi/openmpi-x86_64 ]
      - [ nvhpc/24.3, nvhpc/24.3 ]
      - [ nvhpc/24.5, nvhpc/24.5 ]
      - [ nvhpc/24.7, nvhpc/24.7 ]

script: |
  module load #{load_modules}

As shown in the following screenshot, even if multiple items are selected, they are not reflected in the text area on the right.
a

The expected behavior is that the selected values are correctly expanded and shown in the Script Content.
b

In addition, the behavior of 2.8. path also seems incorrect:

form:
  working_dir:
    widget: path
    label: Working Directory
    show_files: false
    favorites:
      - /lvs0/rccs-aot

script: |
  cd #{working_dir}

If a directory is selected, and after that a different directory is selected again, the updated path is not reflected in the text area.
c

I apologize for not providing test files, but before creating a PR, please make sure that all examples described in the following manual work correctly:
https://riken-rccs.github.io/OpenComposer/docs/application.html

Thank you for your understanding.

@mnakao
Copy link
Contributor

mnakao commented Jan 28, 2026

Addition: In 2.1. number, when I delete the number on the left, its line on the right should disappear, but they don't. It seems the initial value remains.

form:
  nodes:
    widget:   number
    label:    Number of nodes (1 - 128)
    value:    4
    min:      1
    max:      128
    step:     1
    required: false
    help:     The larger the number, the longer the wait time.
    
script: |
  #SBATCH --nodes=#{nodes}
e

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.

2 participants