-
Notifications
You must be signed in to change notification settings - Fork 8
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
Added run_jobs_using_params() #36
Conversation
@mitkotak I have two tasks on this PR:
Thanks! |
Just tested this functionality out, It's pretty cool! There are a few changes we need to make though. I think the way this will be used is to test a variety of parameters/settings to ensure that the job works, so reading from one hard-coded file isn't going to work. I think we should probably remove the code that reads from the JSON if the file exists. Ideally the solution we want is for the user to provide a path to a JSON or a JSON loaded as a dict to a function like "run_job_using_params(...)" that takes in the dictionary of parameters and submits the job. I don't think we need to add it to the UI right now because this is mostly for model developers and the important part is that the functionality exists. Other notes for when we come back to this functionality/future work (aka you don't need to do this 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.
Overall the code is good and seems to work (ran hello world and pysal-access, then restored the job from the UI to check on them). We need to meet with Drew though to figure out how this would be used and if this would be enough. I'm assuming that we will at the very least need to give indicators on the status of the jobs (running/error/completed) or something for this to be useful for end-users.
hpcUsername=None, | ||
hpcPassword=None, | ||
localExecutableFolder={"type": "git", | ||
"gitId": "hello_world"}, |
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.
It seems this is how the user would pass in the "job"/"model" which will probably not be intuitive to them.
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.
Yup I will handle that in ParamAccumulator (Will maybe change the name to something else since its handling "job"/"mode" preprocessing as well).
param_acc = ParamAccumulator(params) | ||
job = self.create_job(maintainer, hpc, hpcUsername, hpcPassword) | ||
job.set(localExecutableFolder, localDataFolder, localResultFolder, param_acc.params, env, slurm) | ||
job.submit() |
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 makes sense and works (tested it with a couple of jobs), but we may also need to report the status of jobs. I imagine the way this is going to be used, end-users will want to know that all of their jobs ran and didn't throw errors. Maybe next week we can sit down with Drew and get feedback on that.
@@ -18,6 +18,12 @@ | |||
from IPython.display import display, Markdown, Javascript | |||
|
|||
|
|||
class ParamAccumulator: |
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.
Not quite understanding what the role of this helper class is at this point. It seems to be a wrapper around a dictionary that doesn't provide any additional functionality.
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.
Yeah the role for that class is to handle all of the input preprocessing that might be needed. For e.g. json paths ( working on it rn ) or some other input format that we might wanna support.
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.
Tested and seems to work. Not sure how this slipped through the cracks, but approving now.
Allows us to do something like