For batched builds, use the whole node#5003
Conversation
GMAKE_J is generally used as a way to throttle builds so as to not overwhelm a login node. There is no such concern for a batched build, so just use the entire node. Also, minor cleanup of batched build resource calculation in test_scheduler.
There was a problem hiding this comment.
Pull request overview
Adjusts TestScheduler’s build resource heuristics so batched builds can use the full compute node (rather than being throttled by login-node-oriented GMAKE_J defaults), and slightly simplifies batched-build resource accounting.
Changes:
- Treat batched builds as low “local” cost by setting
_model_build_cost = 1whenBATCHED_BUILDis enabled. - In
_xml_phase, setGMAKE_JtoMAX_TASKS_PER_NODEfor batched builds (while still cappingGMAKE_Jtoproc_poolfor small-core systems in the non-batched path). - Simplify
_get_procs_neededforMODEL_BUILD_PHASEto always return_model_build_cost.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| from unittest import mock | ||
|
|
||
| from CIME import test_scheduler |
| def test_no_batch_build_init_param_sets_attribute(self): | ||
| """TestScheduler.__init__ sets _batched_build=False when no_batch_build=True.""" | ||
| # We test the init logic in isolation by checking the attribute assignment | ||
| # path (lines 250-251 of test_scheduler.py) with a minimal mock. | ||
| with mock.patch.object( |
| # Make sure ninja exe works! | ||
| nstat, _, nerr = run_cmd(f"{ninja_path}/ninja --version") | ||
| if nstat != 0: | ||
| logger.warning( | ||
| f"Ninja exe does not appear to be usable: {nerr}\nFalling back to gmake" | ||
| ) | ||
| else: | ||
| cmake_args += " -GNinja " | ||
| cmake_env += "PATH={}:$PATH ".format(ninja_path) |
|
Sorry for the churn. This is ready for final review now. |
| check_ninja(case) | ||
| ninja = False |
There was a problem hiding this comment.
Is this correct? It looks like check_ninja returns a logical, so I would expect something like ninja = check_ninja(case).
Either way, a comment would be helpful here. The current code is definitely unintuitive, even if it's correct: why do you set ninja = False if it's originally True? Even with my suggested change, a comment would help clarify what's going on... something like (if I'm understanding correctly): "User has requested a ninja-based build, but we need to check that this is possible; if not, we'll fall back to a non-ninja build"
There was a problem hiding this comment.
Good catch. Not sure what I was thinking here.
| else: | ||
| # We cannot know if the various buildlib scripts support ninja. | ||
| # Just set an env var to let them know ninja was requested. | ||
| os.environ["CIME_SHAREDLIB_NINJA"] = "TRUE" |
There was a problem hiding this comment.
This is definitely unintuitive, even if it's correct. It looks like this says: if ninja is false (so we're in the else block) then we're setting CIME_SHAREDLIB_NINJA to TRUE? That feels backwards.
My expectation is that this whole block would look like:
if ninja:
# Explanatory comment here...
ninja = check_ninja(case)
# If ninja is still true after the check....
if ninja:
# We cannot know if the various buildlib scripts support ninja.
# Just set an env var to let them know ninja was requested.
os.environ["CIME_SHAREDLIB_NINJA"] = "TRUE"If I'm wrong and the current logic is correct, it warrants some comments explaining what right now feels like unintuitive behavior.
billsacks
left a comment
billsacks
left a comment
There was a problem hiding this comment.
Looks good now - I like the way you have set up the logic.
GMAKE_J is generally used as a way to throttle builds so as to not overwhelm a login node. There is no such concern for a batched build, so just use the entire node.
Also, minor cleanup of batched build resource calculation in test_scheduler.
Also, add a check that the ninja exe is usable if the user asked for it.
Also, add support for communicating to sharedlib buildlib that ninja was requested.
Checklist