-
Notifications
You must be signed in to change notification settings - Fork 93
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
Tui 1.0 #5731
Tui 1.0 #5731
Conversation
25548da
to
931d025
Compare
@@ -0,0 +1,31 @@ | |||
<pre><span style="color:#000000;background:#e5e5e5"> </span> |
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 was really hoping GitHub would render this HTML which would make it much easier to review screenshot updates in PRs. Any ideas?
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.
The best thing I can come up with is to use some Python lib to render them into png derived files.
Nice, looking forward to this! |
931d025
to
ad7cbb7
Compare
11926b2
to
5869bd9
Compare
@@ -15,34 +15,37 @@ | |||
# You should have received a copy of the GNU General Public License | |||
# along with this program. If not, see <http://www.gnu.org/licenses/>. | |||
|
|||
"""cylc tui WORKFLOW | |||
"""cylc tui [WORKFLOW] |
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.
If you give a workflow, tui will startup, pre-filtered to just that workflow. Press "E" to unset this filtering an view all workflows.
# auto_add=False, NOTE: at present auto_add can not be turned off | ||
color=False | ||
) | ||
|
||
parser.add_option( | ||
'--display', |
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.
Removed the HTML testing mode from the Tui script as this is now handled by an integration test fixture.
# the UI update interval | ||
# NOTE: this is different from the data update interval | ||
UPDATE_INTERVAL = 0.1 |
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 means that the UI will look for updates every 0.1 seconds, but the frequency of these updates is determined by the updater which is decoupled and runs to its own clock.
# for now we will exit tui when the workflow is cleaned | ||
# this will change when tui supports multiple workflows | ||
cli_cmd('clean', workflow) | ||
sys.exit(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.
Tui used to exit when you cleaned the workflow, but now it can stay open.
# the maximum time to wait for a workflow update | ||
CLIENT_TIMEOUT = 2 |
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.
If the workflow takes more than 2 seconds to send its response, then it will time out. Tui will put a message under the workflow saying that it has timed out, but will retry with the next update cycle.
Previously both the application and updater used the same update period of 1 second. If it took longer to get the response and do the required processing, then Tui would crash.
# the interval between workflow data updates | ||
BASE_UPDATE_INTERVAL = 1 |
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 is the desired update interval. If it takes longer than 1 second to update the workflow, no problem, the updates just come through slower, no more crashes.
c3133b5
to
cb2ac47
Compare
With Tui now better tested, this brings the project coverage up above 90%! We still can get more of Tui covered now there's a testing framework for it. |
2b969af
to
54356aa
Compare
Amazing work 🚀 |
(added some extra tests to fill some coverage holes) |
Good spot, the tests cover toggling workflow states using |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Not unreasonable, I think that's just It might be considered a little dangerous having a button like that kicking about, so we might want to put an "are you sure" check on that which Tui doesn't currently have an interface for. |
Fixed the "p" shortcut and added it to the tests: https://github.com/cylc/cylc-flow/compare/8e371542150b9792a8593399cefc14f96be5c1df..54b7e609af8676c7fcb3b4e8723a50d92ad9c3a7 |
* Fixes an obscure issue where the applications main loop becomes suspended as the result of attempting to register an `on_press` callback which takes as an argument a Cylc client. * Even though this callback is not actually called at this point, just passing the reference to it appears to be enough to cause thread deadlocking because Cylc clients are async based and not thread safe.
Fixed an issue with "E" not working properly: https://github.com/cylc/cylc-flow/compare/54b7e609af8676c7fcb3b4e8723a50d92ad9c3a7..6f3f1701c1106ecd779b11b08ad4293186678da0 The "E" (reset all workflow filters) binding is the one thing in Tui which we cannot test. This is because in order to prevent Tui from seeing other workflows on the system (outside of the test), we have to use an ID filter. Pressing "E" would reset that filter getting Tui to view everything. |
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.
- Functional testing of PR.
- Read function code.
- Read the testing.
I really enjoyed working through the Raikura test framework. 🐦 !
* Post-refactor clean up of dangling code.
Training day came around again, added support for @wxtim, could you check the last three commits. |
@@ -1380,7 +1380,7 @@ def insert_job(self, name, cycle_point, status, job_conf): | |||
name=tproxy.name, | |||
cycle_point=tproxy.cycle_point, | |||
execution_time_limit=job_conf.get('execution_time_limit'), | |||
platform=job_conf.get('platform')['name'], | |||
platform=job_conf['platform']['name'], |
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.
Unrelated, spotted during testing.
The .get
here wasn't protecting against anything as the default return value of None
would cause the ['name']
bit to error anyway.
* Add a log view to tui and add it to the context menu for workflows, tasks and jobs. * Allow raikura sessions to see module-scoped fixtures when appropriate. * Fix a bug (which only surfaced in tests for some reason) where a colour was specified as an empty string which urwid doesn't appear to like.
* Add `cylc show` support for tasks in Tui.
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 is great. Please stop with the mission creep so that we can merge. Pretty Please.
Remove the "Tui is experimental" notice 🎉
scan
pipe to list workflows in Tui the same way we use it in the CylcUIServer.cylc cat-log
.cylc show
.tui2-large.mp4
In the process I spotted an issue with task filtering which looks like it might be a data store thing but requires more investigation, lower priority as filtering is not an issue for the GUI. Until then the task filtering won't work right: #5716
TODO:
Tui integration test failures result in HTML files like this being written into the test directory in
~cylc-run/
. When tests fail in CI, these HTML files will be bundled into the artefact so can be inspected locally to determine the cause of the failure:Check List
CONTRIBUTING.md
and added my name as a Code Contributor.setup.cfg
(andconda-environment.yml
if present).CHANGES.md
entry included if this is a change that can affect users?.?.x
branch.