-
Notifications
You must be signed in to change notification settings - Fork 5
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
Adding options to reduce prints #146
Conversation
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.
Looks like it works for me, but want @liu15 to agree.
ats/machines.py
Outdated
@@ -317,7 +317,8 @@ def testEnded(self, test, status): | |||
#note test.status is not necessarily status after this! | |||
#see test.expectedResult | |||
|
|||
self.noteEnd(test) #to be defined in children | |||
if not configuration.options.removeEndNote: | |||
self.noteEnd(test) #to be defined in children |
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 this is correct because some machines (slurm, flux, and I think lsf) use noteEnd to calculate the number of remaining resources. The implementations of noteEnd need to be refactored to remove the print portion (perhaps as logEnd).
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.
Check, thanks for that @liu15
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.
you are correct Ben. That needs fixed.
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.
See Ben's comment. noteEnd needs called in order to track machine resources.
ats/machines.py
Outdated
if not configuration.options.removeEndNote: | ||
self.noteEnd(test) #to be defined in children | ||
print(endNote) |
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.
Can you protect this with an
if endNote:
so that if noteEnd does not return a string, nothing is printed?
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.
Yes, I can do that. Was also working on a print for ATS' flux module to be consistent with the other schedulers.
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.
Approved, merge is fine by me
This PR adds two options:
--removeStartNote
and--removeEndNote
removeStartNote: removes the logStart() call from step(). This gets rid of the "Start" message automatically printed by ATS
removeEndNote: removes the noteEnd() call from testEnded(). This gets rid of the "Stop" message automatically printed by ATS
Additionally, this PR adds a message printed at the end of tests when they are run using flux, to be consistent with the other schedulers.