-
Notifications
You must be signed in to change notification settings - Fork 37
Refine SmartSimEntity Interface #688
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
Refine SmartSimEntity Interface #688
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## smartsim-refactor #688 +/- ##
=====================================================
+ Coverage 40.45% 43.36% +2.90%
=====================================================
Files 110 110
Lines 7326 7042 -284
=====================================================
+ Hits 2964 3054 +90
+ Misses 4362 3988 -374
|
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.
A couple of small testing nits and requests for some docs, but nothing that should take long to address! Looks about ready to go on my end!!
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.
Just a couple of comments/questions
smartsim/_core/control/job.py
Outdated
@@ -299,7 +299,7 @@ def error_report(self) -> str: | |||
warning += f"Job status at failure: {self.status} \n" | |||
warning += f"Launcher status at failure: {self.raw_status} \n" | |||
warning += f"Job returncode: {self.returncode} \n" | |||
warning += f"Error and output file located at: {self.entity.path}" | |||
# warning += f"Error and output file located at: {self.job.path}" |
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.
Is this commented out because it wrong or because it just won't work?
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.
Path is no longer on entity, for a second I thought I could access off of Job but no. I commented it out and left it incase we want to replace in the future, it will not be forgotten
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 last nit, but nothing to hold approval up over! LGTM!!
4a4f43a
into
CrayLabs:smartsim-refactor
Refactor SmartSimEntity class, remove ExecutableProtocol protocol, Application becomes subclass of ABC SmartSImEntity