-
Notifications
You must be signed in to change notification settings - Fork 2
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
Make slurm jobs respect --match #64
Conversation
damnit/backend/extract_data.py
Outdated
@@ -204,6 +204,10 @@ def extract_and_ingest(self, proposal, run, cluster=False, | |||
|
|||
python_cmd = [sys.executable, '-m', 'damnit.backend.extract_data', | |||
'--cluster-job', str(proposal), str(run), run_data.value] | |||
if len(match) > 0: | |||
for m in match: |
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.
should you also check that any of the match is actually a cluster variable and only start the job in this case?
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.
That's already done a couple lines above:
# Launch a Slurm job if there are any 'cluster' variables to evaluate
ctx = self.ctx_whole.filter(run_data=run_data, name_matches=match, cluster=cluster)
ctx_slurm = self.ctx_whole.filter(run_data=run_data, name_matches=match, cluster=True)
This change is only to forward those match arguments to the slurm job.
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.
I see, thanks for the clarification. LGTM
damnit/backend/extract_data.py
Outdated
@@ -204,6 +204,10 @@ def extract_and_ingest(self, proposal, run, cluster=False, | |||
|
|||
python_cmd = [sys.executable, '-m', 'damnit.backend.extract_data', | |||
'--cluster-job', str(proposal), str(run), run_data.value] | |||
if len(match) > 0: |
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.
I don't think you need this condition - if m
is empty, the loop won't do anything anyway. 🙂
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.
Ah true, removed in abf6e13.
LGTM too |
Previously the --match arguments weren't passed to the slurm job, so all variables would be reprocessed.
a6e30fa
to
abf6e13
Compare
Previously the --match arguments weren't passed to the slurm job, so all variables would be reprocessed.
(cherry-picked from one of my giant branches)