Skip to content

Retire launch script #62

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.

Sign up for GitHub

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

Jump to bottom
Merged
XkunW merged 29 commits into from
Mar 13, 2025
Merged

Retire launch script #62

XkunW merged 29 commits into from
Mar 13, 2025

Conversation

@XkunW
Copy link
Contributor

@XkunW XkunW commented Mar 12, 2025

PR Type

[Feature | Fix]

Short Description

  • Retire launch_server script, the launch command now takes care of setting environment variables and runs the sbatch command directly
  • Updated default behaviour for launching a model that's absent from config, only send a warning msg if the model weights exist
  • Replaced all os.path methods with pathlib
  • Moved singularity containers to model-weights and added a cache config
  • Added a json file to keep track of launch params and server address for each job
  • Updated logging file structure
  • Updated CLI integration tests to accommodate the changes, split the launch_command_model_not_found test into two
  • Minor updates to various variable names
  • Fixed Qwen2.5-Math model config issues

Tests Added

test_launch_command_model_not_in_config_with_weights
test_launch_command_model_not_found

XkunW and others added 29 commits February 28, 2025 16:59
@XkunW
Rename num_gpus to gpus_per_node to avoid confusion
72618f2
@XkunW
Move launch server logic to LaunchHelper, display a warning message i…
6e6617f 
…nstead of raising an exception if the model weights exist but missing relevant config
@XkunW
Point singularity path to new location, dump server addr to server js…
86e7026 
…on file, update env var name
@XkunW
Update os.path methods to pathlib, add second returned var for run ba…
3ad0ea1 
…sh command, update CACHED CONFIG location
@XkunW
Add json handling for read slurm log, update get_base_url to read fro…
c2f17c9 
…m launch json file
@XkunW
Remove SERVER_ADDRESS_SIGNATURE
057d683
@XkunW
Update launch logic to match launch helper, add error check for runni…
38d4245 
…ng bash command
@XkunW
ruff format
b9e166f
@XkunW
Typing fixes
4568019
@XkunW
Change the default value for log dir, update num_gpus to gpus_per_node
95be110
@XkunW
Remove redundant cuda loading commands (legacy from venv), fix server…
54dda51 
… addr writing, update binding to the specific model weights dir
@XkunW
Replace NUM_GPUS with SLURM_GPUS_PER_NODE
43324ba
@XkunW
Replace NUM_NODES with SLURM_JOB_NUM_NODES
3e0393c
@XkunW
Various small fixes for launch command, removed unnecessary env vars,…
3fc6120 
… updated logging structure
@XkunW
Update log file path to match changes in _helper
0fa5d20
@XkunW
Update server address key for launch json
3d6a64e
@XkunW
Update err messages, update util tests to match changes in _utils
c6c08f6
@XkunW
Move convert bool val to utils
8aa76cf
@XkunW
Fix max_model_len for Qwen2.5-Math models, added Qwen2.5-Math-PRM-7B …
65258c9 
…model
@XkunW
Rename cached config
0bae6fd
@XkunW
Update warning msg, remove model name exclusion from params, update p…
3f32196 
…rivate method
@XkunW
Fix & update launch command integration tests
5193157
@XkunW
ruff format
0e4fc09
@XkunW
Add helper for list command
4dda406
@XkunW
Retire launch script
0dbf73c
@pre-commit-ci
[pre-commit.ci] Add auto fixes from pre-commit.com hooks
8005f95 
for more information, see https://pre-commit.ci
@XkunW
Merge branch 'retire_launch_script' of https://github.com/VectorInsti…
aa7325b 
…tute/vector-inference into retire_launch_script
@XkunW
Fix mypy and ruff errors for test_cli.py
da39919
@XkunW
mypy fixes
4cb0552
@jwilles jwilles self-assigned this Mar 12, 2025
@jwilles jwilles requested review from amrit110, fcogidi and jwilles March 12, 2025 21:04
@jwilles jwilles removed their assignment Mar 12, 2025
amrit110
amrit110 approved these changes Mar 13, 2025
View reviewed changes
Copy link
Member

@amrit110 amrit110 left a comment

Choose a reason for hiding this comment

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

Looks great. I assume it has been tested on the cluster too, I'm going to do that as well. Great refactor!

@XkunW XkunW merged commit 4a9b41c into develop Mar 13, 2025
4 checks passed
@XkunW XkunW deleted the retire_launch_script branch March 13, 2025 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@amrit110 amrit110 amrit110 approved these changes

@jwilles jwilles Awaiting requested review from jwilles

@fcogidi fcogidi Awaiting requested review from fcogidi

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@XkunW @amrit110 @jwilles