-
Notifications
You must be signed in to change notification settings - Fork 37
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
RunSettings Set Method #166
Conversation
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.
Couple changes. Let me know your thoughts on the condition
flag
Merge remote-tracking branch 'upstream/develop' into rs-set-arg
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.
Minor comments, mostly on docstrings. Great stuff!
smartsim/settings/base.py
Outdated
rs = RunSettings("python") | ||
rs.set("an-arg", "a-val") | ||
rs.set("a-flag") | ||
rs.run_args["an-arg"] # returns "a-val" |
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.
Ideally users shouldn't know that run_args
exist now and solely use .set()
if you want to show the arguments in the docstring then lets call a user facing method like RunSettings.format_run_args
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.
Does this mean that we should be moving run_args
-> _run_args
or is that considered out of scope for this PR?
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.
Leaving as is for now, fixed docstr in 4a10a76
smartsim/settings/base.py
Outdated
"""allows users to set individual run arguments. Does | ||
basic formating such as stripping leading dashes. | ||
|
||
This method also ensures that the argument being set |
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.
So we don't actually check that it IS valid. Lets just state that condition can be used to write scripts that will adapt under variables that can be changed at runtime. like, for example, the hostname one.
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.
Updated in 4a10a76
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.
LGTM! Thanks for making those changes!
This PR implements a
RunSettings.set
which is able to programmatically set run arguments:run_args
dictionary.RunSettings
are able to further restrict arguments that may conflict with SmartSim's process by providing their own restricted arguments.Looking for feedback and/or additional arguments that should be restricted in the subclasses.