Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[develop] First implementation of run_WE2E_tests.py #558
[develop] First implementation of run_WE2E_tests.py #558
Changes from 31 commits
6846d25
f9fdd50
24e3b63
a912c39
17de277
d3cca9f
e7ed371
b36bb7d
8ae8327
2e614e3
7d7501f
3cce1d7
2274c0d
3d53ed7
ef072f9
d2a4cac
18e1355
39083c7
c62c8ce
374ed9e
f439b63
e48fc4b
3774fa6
ee995fb
e447cba
64944eb
213720d
786ad00
05c06b7
d0c7a17
faa711a
f6bd027
cc9ffb6
f4a0b0a
3145ee0
70a477d
74d9608
dd86024
61c5940
cfaca28
3dee9a3
8378507
5bcd011
cbcfe5c
a6e6608
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
task[2]
here depends on yourSELECT
statement never changing. It might be helpful for the user if you made a named tuple to hold the information in the task so that you don't have to worry about order here and losing track.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 do not see a way to convert the output from fetchall() to a list of namedtuples without significantly increasing the complexity of the code. I'd rather just handle this with a comment.
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.
Will the calls to subprocess like this wait until
rocotorun
completes before control is returned to the Python script? If not, you might be over-taxing the system here.Also, is
rocotorun
also running in a cron job at this point?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.
subprocess.run
waits for the subprocess to complete before continuing (python documentation here). I decided not to try to capture the output from the subprocess because this seems to slow things down drastically (like, orders of magnitude slower).The
monitor_jobs()
function is only called byrun_WE2E_tests.py
if the--use_cron_to_relaunch
option is not specified, so this should never be run in conjunction with crontab running. But I can't control what users choose to do; theoretically this should work just fine if someone also had a crontab entry for this job and called this script manually.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.
Any chance this is a problem if a user calls this script for a real-time experiment whose first cycle is a LONG time ago and the cycles since then are all inactive?
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 script is not designed with real-time experiments in mind, just the WE2E tests. If realtime functionality is desired and the current logic is insufficient then those considerations can be taken into account in future updates.
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.
How should this file be updated when changes are made to monitor_jobs.py such that this file is modified?
Can that be a GitHub action?
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 is unclear to me how that would be handled in an automatic way. Is it worth the effort for something that will likely not change very often, if at all?
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 could be nice just to drop in a comment on how this appeared here so that future you is not completely baffled. ;)