-
Notifications
You must be signed in to change notification settings - Fork 110
Add max_memory keyword to the Everest config #10637
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.
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
base: main
Are you sure you want to change the base?
Conversation
40969d6
to
b0f8315
Compare
CodSpeed Performance ReportMerging #10637 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I have one question that is slightly more work, but think it might be worth it in the long run, not sure what you think?
@field_validator("max_memory") | ||
@classmethod | ||
def validate_max_memory(cls, max_memory: int | str | None) -> str | None: | ||
if max_memory is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reuse this?
ert/src/ert/config/queue_config.py
Line 364 in 99d7804
def _parse_realization_memory_str(realization_memory_str: str) -> int: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
@@ -58,6 +59,29 @@ class SimulatorConfig(BaseModelWithContextSupport, extra="forbid"): | |||
A value of 0 means unlimited runtime. | |||
""", | |||
) | |||
max_memory: int | str | None = Field( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would entail a bigger refactor, which would also touch ert, but there is a concept in ert of generic queue options, and I think this one probably belongs with that
ert/src/ert/config/queue_config.py
Line 40 in 99d7804
class QueueOptions( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I noted that also. It seems not everything lives in there though, for instance the QueueOptions
base class does contain max_running
, but not max_runtime
, or realization_memory
, they live in QueueConfig
. Maybe realization_memory
does belong in the options class since it is basically a property you set in the node. But maybe that is true for some of the other fields too? Changing that should maybe a separate issue since it touches ERT.
Also, then the question is if it changes the Everest config: Do we keep max_memory
as a field in SimulatorConfig
? Or does the user have to specify it in the queue-specific options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is somewhat related to this issue #10628. If we had the same keyword for max memory for everest and ert it would simplify giving the user error messages like this.
3383365
to
cd96e46
Compare
cd96e46
to
d431fb3
Compare
Issue
Resolves #10624
Resolves #9569
Approach
Add the keyword, add to generated ert config
(Screenshot of new behavior in GUI if applicable)
git rebase -i main --exec 'just rapid-tests'
)When applicable