Skip to content

Make slurm workflow manager call execute_experiment#1589

Draft
rfbgo wants to merge 2 commits into
GoogleCloudPlatform:developfrom
rfbgo:slurm_ee
Draft

Make slurm workflow manager call execute_experiment#1589
rfbgo wants to merge 2 commits into
GoogleCloudPlatform:developfrom
rfbgo:slurm_ee

Conversation

@rfbgo

@rfbgo rfbgo commented May 28, 2026

Copy link
Copy Markdown
Collaborator

Placeholder for discussion of changing slurm workflow manager to use execute_experiment

@rfbgo rfbgo marked this pull request as draft May 28, 2026 15:49

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the Slurm workflow manager template to source an execute_experiment script instead of running a generic command, and updates the corresponding tests to verify the generation and contents of this script. The review feedback suggests quoting the {execute_experiment} path in the template to prevent word splitting, and explicitly specifying encoding="utf-8" when opening files in the test suite to ensure cross-platform consistency.

Comment thread var/ramble/repos/builtin/workflow_managers/slurm/slurm_experiment_sbatch.tpl Outdated
Comment thread var/ramble/repos/builtin/workflow_managers/slurm/test/slurm.py
@ramble-pr-bot

ramble-pr-bot Bot commented May 28, 2026

Copy link
Copy Markdown

Ramble Performance Test Metrics

Results produced with commit: 5605f32

Test Name Outcome Duration (s) Last 5 Avg (s)
test_analyze_large_file passed 3.0322 2.9806
test_large_template_expansion passed 2.2269 2.1303
test_many_experiments passed 78.4217 77.7977
test_many_objects_defaults passed 32.0101 34.9550
test_matrix_filter_perf passed 1.6304 1.6242

…ent_sbatch.tpl

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
@codecov

codecov Bot commented May 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.90%. Comparing base (20f49f7) to head (5605f32).
⚠️ Report is 5 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1589   +/-   ##
========================================
  Coverage    92.90%   92.90%           
========================================
  Files          340      340           
  Lines        32580    32584    +4     
========================================
+ Hits         30269    30273    +4     
  Misses        2311     2311           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

{workflow_hostfile_cmd}

{command}
. "{execute_experiment}"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a great idea!

One thing I wonder, is if (say Slurm) user has expectations that custom sbatch headers they added into execute_experiment template should work, and maybe surprised that they don't?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yea great call out, this is definitely a very real and valid concern. Do you think it is sufficient to detect it and warn the user? or is it so concerning that we should not pursue this approach?

I guess it's also a risk they do this now into execute_experiment and not realize it is potentially unused

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is the right approach, but a warning might be useful.

We also have an existing warning (via the workflow_banner), that's likely made obsolete with the current change.

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