Skip to content

Fix v4-megatron-sakura installer #84

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix v4-megatron-sakura installer #84

wants to merge 1 commit into from

Conversation

odashi
Copy link
Member

@odashi odashi commented Jun 9, 2025

  • PBS変数を run_setup.sh 側に寄せる
  • 予約IDを引数化しスクリプトから削除

@odashi odashi requested review from hkiyomaru and Copilot June 9, 2025 05:03
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the installer scripts by moving PBS options into the submission wrapper, parameterizing the reservation ID, and cleaning up the helper script.

  • Require a second argument (reservation-id) in run_setup.sh and pass it to qsub
  • Relocate PBS flags (-P, -q, -o, -e, -l) into run_setup.sh
  • Strip all #PBS directives from qsub_setup.sh

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
pretrain/installers/v4-megatron-abci/run_setup.sh Added reservation-ID argument, moved PBS flags into qsub command
pretrain/installers/v4-megatron-abci/qsub_setup.sh Removed #PBS directives now managed externally
Comments suppressed due to low confidence (3)

pretrain/installers/v4-megatron-abci/run_setup.sh:11

  • Quote variable expansions (e.g., use "${target_dir}" and "${reservation_id}") when passing to qsub flags to prevent issues with spaces or special characters.
target_dir=$1; shift

pretrain/installers/v4-megatron-abci/run_setup.sh:8

  • [nitpick] Indent the exit 1 line inside the conditional block consistently with the surrounding echo statements for better readability.
exit 1

pretrain/installers/v4-megatron-abci/run_setup.sh:1

  • [nitpick] Consider adding a script header comment that documents the new reservation-id parameter and the purpose of the script.
#!/bin/bash

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.

1 participant