-
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
Improve parameter handling with and without generation #107
Conversation
A few questions about this implementation
My main concern is that this isn't entirely straightforward given the naming convention. Alternative approaches to consider:
thoughts on those? Either way I think |
Thanks for the review, @Spartee! I agree, naming is confusing. I think it is fine to just call all of them I'll adapt the code in the following way:
The only minor drawback is that |
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.
one comment but otherwise LGTM
if not param in self.params: | ||
raise SSConfigError(f"Tried to convert {param} to command line argument " + | ||
f"for Model {self.name}, but its value was not found in model params") | ||
if self.run_settings 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.
will this case ever happen?
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.
@Spartee it seems like we don't check whether run_settings
is None
, thus a very bad user might initialize a Model
with run_settings=None
. I don't know if there is any reason why we are allowing this, otherwise we might want to guard against it in another part of the code?
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.
yea I think step creation would just fail, but when we go to the server arch I think thisll be more important.
This PR enhances the current way parameters and models are generated in two ways:
arg_params
inEnsemble
s:arg_params
are treated likeparams
but set asexe_args
in the createdModel
s -- thus they are set even without the generation step.This PR resolves #16 #40