-
Notifications
You must be signed in to change notification settings - Fork 21
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
Define output dict for multi species in main.py #735
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #735 +/- ##
==========================================
- Coverage 73.82% 73.81% -0.01%
==========================================
Files 99 99
Lines 27346 27352 +6
Branches 5717 5718 +1
==========================================
+ Hits 20187 20189 +2
- Misses 5733 5737 +4
Partials 1426 1426
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Thanks! Can you please see the two comments below?
@@ -180,6 +180,8 @@ class ARC(object): | |||
Job types not defined in adaptive levels will have non-adaptive (regular) levels. | |||
output (dict): Output dictionary with status and final QM file paths for all species. Only used for restarting, | |||
the actual object used is in the Scheduler class. | |||
output_multi_spc (dict): Output dictionary with status and final QM file paths for the multi species. |
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.
please also add above under Args:
@@ -291,6 +294,7 @@ def __init__(self, | |||
if not os.path.exists(self.project_directory): | |||
os.makedirs(self.project_directory) | |||
self.output = output | |||
self.output_multi_spc = output_multi_spc |
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.
we store it as an attribute, but we don't do anything with it. I think we should transmit it to Scheduler (and also add it as an argument in Scheduler like here)
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.
We actually transmitted it to Scheduler as an attribute in the previous PR already (like what had been done to the output dict), I think we just forgot to define it in the main.py.
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.
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.
Also, do we need it part of this function for the dumping of the yaml? @alongd
https://github.com/ReactionMechanismGenerator/ARC/blob/688f9602e7d41252cc0e22a27cd30a4d59eafcf5/arc/main.py#L464C1-L467C12
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 could be misunderstanding but I think @alongd meant here
Line 591 in 688f960
self.scheduler = Scheduler(project=self.project,
Thank you for pointing that out. Indeed, we didn't transmit the output dictionary to the Scheduler. I have a question, though. I thought output_multi_spc
was the multi-species counterpart to output
. However, since we didn't transmit output
here, why is there a need to transmit output_multi_spc
? Am I missing something?
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 maybe you're right. I'm not really familiar with the restart function. Do you know with these changes you've made that if a restart of a multi species works? As in ARC picks up that a multi species is/was run(ning)
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'm not that familiar with the restart function either. Currently, I only have restart.yml
from completed projects (both single and multi-species), which have already finished running. With these changes, I launched restart.yml
for single and multi-species projects individually and compared them. They do yield similar folders, such as log_and_restart_archive
, and no errors were reported in the err.txt
. However, I'm not sure how this will turn out in the future with an unfinished project.
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.
Areyou able to do with it unfinished. What I mean is start the job with the input.yml and then as soon as you see it submits its first job to the server, cancel ARC qdel <job id>
and then restart it
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.
Now I get the following error, this is the error when I terminate the ARC submitted job
Traceback (most recent call last):
File "/home/jintaowu/Code/ARC/ARC.py", line 69, in <module>
main()
File "/home/jintaowu/Code/ARC/ARC.py", line 65, in main
arc_object.execute()
File "/home/jintaowu/Code/ARC/arc/main.py", line 620, in execute
skip_nmd=self.skip_nmd,
File "/home/jintaowu/Code/ARC/arc/scheduler.py", line 508, in __init__
self.schedule_jobs()
File "/home/jintaowu/Code/ARC/arc/scheduler.py", line 595, in schedule_jobs
successful_server_termination = self.end_job(job=job, label=label, job_name=job_name)
File "/home/jintaowu/Code/ARC/arc/scheduler.py", line 993, in end_job
self._run_a_job(job=job, label=label)
File "/home/jintaowu/Code/ARC/arc/scheduler.py", line 1060, in _run_a_job
xyz=job.xyz,
File "/home/jintaowu/Code/ARC/arc/scheduler.py", line 801, in run_job
checkfile = self.species_dict[label].checkfile if isinstance(label, str) else None
KeyError: 'multi_spc1'
And this is the error when I restart the job
Traceback (most recent call last):
File "/home/jintaowu/Code/ARC/ARC.py", line 69, in <module>
main()
File "/home/jintaowu/Code/ARC/ARC.py", line 65, in main
arc_object.execute()
File "/home/jintaowu/Code/ARC/arc/main.py", line 620, in execute
skip_nmd=self.skip_nmd,
File "/home/jintaowu/Code/ARC/arc/scheduler.py", line 483, in __init__
self.run_opt_job(species.label, fine=self.fine_only)
File "/home/jintaowu/Code/ARC/arc/scheduler.py", line 1188, in run_opt_job
if self.output_multi_spc[self.species_dict[label].multi_species].get(key, False):
KeyError: 'multi_spc1'
f4f0dd9
to
5343194
Compare
5343194
to
a21c066
Compare
d1aadc1
to
5ada2a6
Compare
d561230
to
fe99b26
Compare
main.py adds `output_multi_spc` as key in ARC.as_dict make output_multi_spc in restart_dict not None
fe99b26
to
d2073a4
Compare
I confirm that ARC's restart does not crash on this branch. @calvinp0, do you have any additional comments, or can we merge? |
The code scanning issues need to be resolved I think |
d2073a4
to
5ca8ae1
Compare
@calvinp0, thanks for pointing it out. @alongd, the updated branch is now more straightforward and robust. If there are any other comments, please feel free to let me know. |
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.
Thanks @JintaoWu98!
We have problem when we launch
restart.yml
saying unexpected keyword argument 'output_multi_spc'. It turns out previously we definedScheduler.output_multi_spc
parallel toScheduler.output
inScheduler
__init__
, however, we forgot to define it in theARC
class inmain.py
. So now we add the relevant terms inmain.py
.