Skip to content

Add dynamic max_timeIDX_offset controlled by CSV and update harness templates#36

Closed
wshlavacek wants to merge 8 commits into
mainfrom
feature/add-max_timeIDX_offset
Closed

Add dynamic max_timeIDX_offset controlled by CSV and update harness templates#36
wshlavacek wants to merge 8 commits into
mainfrom
feature/add-max_timeIDX_offset

Conversation

@wshlavacek
Copy link
Copy Markdown

Yoke training harness to allow dynamic control of the max_timeIDX_offset parameter using a CSV file. Previously, the parameter was hard-coded in the training script. See README.md for full details.

@wshlavacek wshlavacek requested a review from hickmank April 9, 2025 04:25
@galegozi
Copy link
Copy Markdown
Contributor

galegozi commented Apr 9, 2025

Hi @wshlavacek ,
I did not do a full review, but I have a few things to say regarding the PR:

  1. I like what you're doing here (making a parameter available to the user instead of having it fixed in file)!!!
  2. I made a PR (Automating the slurm file generation #12 ) to automate the SLURM file generation, would it be possible to interface with that? (instead of having the SLURM files as part of the harness)? I would be more than happy to help you with this! What I mean by this is can we use the config.json instead of the SLURM files (the training_slurm.tmpl and the training_START.slurm)?
  3. Can we wait until [WIP] START_study.py upgrade #34 is figured out? bc I have some concerns there that would effect this
    Thanks, and good work!

@wshlavacek
Copy link
Copy Markdown
Author

@galegozi
#12 and #34 seem like improvements to me. Fewer files in a harness is better IMHO.

@hickmank
Copy link
Copy Markdown
Collaborator

@galegozi #12 and #34 seem like improvements to me. Fewer files in a harness is better IMHO.

@wshlavacek @galegozi it would be good to use the slurm json config functionality in PR#12. The other PR, modifying START_study.py, has too many details to work out for us to hold this work up for.

Copy link
Copy Markdown
Collaborator

@hickmank hickmank left a comment

Choose a reason for hiding this comment

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

A couple of minor changes requested in comments. It looks good. Let's meet to discuss the final study before we push "go".

Comment thread applications/harnesses/ch_DDP_time_loderunner/README.md
Comment thread applications/harnesses/ch_DDP_time_loderunner/START_study.py
Comment thread applications/harnesses/ch_DDP_time_loderunner/ddp_study_time.csv
Comment thread applications/harnesses/ch_DDP_time_loderunner/training_input.tmpl Outdated
hickmank
hickmank previously approved these changes Apr 16, 2025
@wshlavacek wshlavacek force-pushed the feature/add-max_timeIDX_offset branch from f24f665 to fbe8c3d Compare April 17, 2025 21:52
@wshlavacek wshlavacek requested a review from hickmank April 17, 2025 22:01
Comment thread applications/harnesses/ch_DDP_time_loderunner/archive_runs.sh
Comment thread applications/harnesses/ch_DDP_time_loderunner/check_progress.py
Comment thread applications/harnesses/ch_DDP_time_loderunner/run_yoke_study.sh
@galegozi
Copy link
Copy Markdown
Contributor

Hi @wshlavacek ,
I looked over your PR. I noticed some of your files were included in the harness. I think some of the files should be moved to core Yoke (not in the harness).
What do you think?

Copy link
Copy Markdown
Collaborator

@hickmank hickmank left a comment

Choose a reason for hiding this comment

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

The max_timeIDX_offset change looks fine along with the study that's been set up. I agree with @galegozi comments that some of the scripts you've included provide nice functionality generally so should live somewhere else. I think those should be included in a separate PR and removed from this PR. Open to dissenting opinions though :-)

Comment thread applications/harnesses/ch_DDP_time_loderunner/archive_runs.sh
Comment thread applications/harnesses/ch_DDP_time_loderunner/check_progress.py
Comment thread applications/harnesses/ch_DDP_time_loderunner/run_yoke_study.sh
@hickmank
Copy link
Copy Markdown
Collaborator

hickmank commented May 2, 2025

@wshlavacek where are we at on this? Let me know when I should re-review.

@wshlavacek wshlavacek requested a review from hickmank June 5, 2025 05:28
@wshlavacek
Copy link
Copy Markdown
Author

I created a separate PR for the four helper scripts. I'd like to keep copies of the helper scripts here, for now. I can remove later.

@hickmank
Copy link
Copy Markdown
Collaborator

@wshlavacek can this PR be closed in favor of PR #62 ?

@hickmank hickmank closed this Feb 4, 2026
@hickmank hickmank deleted the feature/add-max_timeIDX_offset branch February 4, 2026 21:10
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