Skip to content

DM-54449: updates for mempercore, peakmemory#48

Merged
daues merged 3 commits intomainfrom
tickets/DM-54449
Mar 26, 2026
Merged

DM-54449: updates for mempercore, peakmemory#48
daues merged 3 commits intomainfrom
tickets/DM-54449

Conversation

@daues
Copy link
Contributor

@daues daues commented Mar 23, 2026

No description provided.

@codecov
Copy link

codecov bot commented Mar 23, 2026

Codecov Report

❌ Patch coverage is 35.00000% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 35.44%. Comparing base (56af205) to head (0815924).
⚠️ Report is 4 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
python/lsst/ctrl/execute/allocator.py 41.66% 8 Missing and 6 partials ⚠️
python/lsst/ctrl/execute/slurmPlugin.py 0.00% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #48      +/-   ##
==========================================
- Coverage   35.68%   35.44%   -0.25%     
==========================================
  Files          14       14              
  Lines         723      759      +36     
  Branches       71       80       +9     
==========================================
+ Hits          258      269      +11     
- Misses        456      475      +19     
- Partials        9       15       +6     

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

Copy link

@MichelleGower MichelleGower left a comment

Choose a reason for hiding this comment

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

Ideally there would be a way to make settings per queue. If need this capability ASAP, should punt that to future ticket. Also, I'm not sure whether this would break if the peak settings don't appear in the site config which would be the case for any other sites using this. It would be nice to have defaults so that it doesn't just break. It would be nice to have documentation with the package some day (instead of only in the developer guide) as well as doc/changes.

action="store",
default=4096,
dest="mempercore",
help="Memory per core to be scheduled by default",

Choose a reason for hiding this comment

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

Put the units in the help message?


def getPeakcpus(self):
"""Accessor for PEAKCPUS
@return the value of PEAKCPUS

Choose a reason for hiding this comment

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

LSST Developer Guide describes the format for docstrings (mostly Numpydoc style)

@return the value of PEAKMEMORY
"""
peakmemory = self.getParameter("PEAKMEMORY")
if self.opts.queue == "torino":

Choose a reason for hiding this comment

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

This conversion seems like it should be in the platform specific package instead of the generic allocator package. Is there a config way to have peak information per queue?

if self.opts.mempercore:
self.commandLineDefaults["MEMPERCORE"] = self.opts.mempercore
else:
self.commandLineDefaults["MEMPERCORE"] = 4096

Choose a reason for hiding this comment

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

Would be nice to not replicate this value here and in the default value for the command line options.

"""Accessor for PEAKMEMORY
@return the value of PEAKMEMORY
"""
peakmemory = self.getParameter("PEAKMEMORY")

Choose a reason for hiding this comment

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

Does getParameter guarantee a value (the default in the pex config is None)

@daues daues merged commit cdaafa6 into main Mar 26, 2026
15 of 17 checks passed
@daues daues deleted the tickets/DM-54449 branch March 26, 2026 00:29
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