Add gui workflow arg #370

datamel · 2022-06-23T09:57:23Z

This adds functionality for opening the gui at a specific workflow when running cylc gui <workflow_name>

These changes close #239 and #240

This PR also restricts the number of gui instances to one, by searching for pid file created by jupyter server.

I have read CONTRIBUTING.md and added my name as a Code Contributor.
Contains logically grouped changes (else tidy your branch by rebase).
Does not contain off-topic changes (use other PRs for other changes).
Any dependency changes applied to setup.cfg. No dependency changes.
Does not need tests (This is inbuilt Jupyter tech (default url)).
Appropriate change log entry included.
I have opened a documentation PR at cylc/cylc-doc/pull/XXXX.
No documentation update required.

datamel · 2022-06-23T12:49:33Z

cylc/uiserver/scripts/gui.py

+            # workflow_id, _, _ = parse_id(
+            # arg, constraint='workflows', infer_latest_runs=True)
+
+            # The following line needs to be replaced with the line above in
+            # order to interpret the workflow_id. This, causes problems with
+            # the ServerApp Event loop.
+
+            workflow_id = arg


PR now interprets workflow_ID given on the command line. e.g. cylc gui workflow_name will resolve correctly to give cylc gui workflow_name/run4.

codecov-commenter · 2022-06-26T22:10:09Z

Codecov Report

Merging #370 (658c7e6) into master (549afbd) will decrease coverage by 0.06%.
The diff coverage is 79.62%.

@@            Coverage Diff             @@
##           master     #370      +/-   ##
==========================================
- Coverage   79.89%   79.83%   -0.07%     
==========================================
  Files          12       12              
  Lines        1159     1210      +51     
  Branches      219      225       +6     
==========================================
+ Hits          926      966      +40     
- Misses        196      206      +10     
- Partials       37       38       +1

Impacted Files	Coverage Δ
cylc/uiserver/app.py	`85.00% <37.50%> (-3.60%)`	⬇️
cylc/uiserver/scripts/gui.py	`87.50% <86.95%> (-12.50%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

oliver-sanders

LGTM, works great

cylc/uiserver/scripts/gui.py

hjoliver

Looks good, tested as working, just a couple of small questions.

cylc/uiserver/app.py

cylc/uiserver/scripts/gui.py

wxtim · 2022-07-26T08:01:16Z

I have picked this up and am running with in an attempt to get it over the line and responded to most of the comments.

I have 2 questions:

Where is the argument documentation coming from?
is it OK that cylc gui --new myflow works but that cylc gui myflow --new doesn't, or is this unexpected?

hjoliver · 2022-07-27T05:45:26Z

Thanks @wxtim - as noted on Element chat, no need to bust a gut if short of time to get this in the release (next release is OK).

Where is the argument documentation coming from?

I haven't had a chance to look today, sorry.

is it OK that cylc gui --new myflow works but that cylc gui myflow --new doesn't, or is this unexpected?

Personally I think options should come before args (otherwise, logically, it sort-of looks like --new is an option for the arg rather than the command). However, some (most?) programs are flexible on that, and in the cylc CLI is, so ideally this sub-command should be too for consistency. Whether or not that's feasible is another matter, given cylc gui starts jupyter server...

oliver-sanders · 2022-07-27T07:05:06Z

The CLI docs are coming from Jupyter Server, options are configured "traits", these double as both CLI options and configurations. E.G. the CylcUIServer adds a trait called --ui-build-dir which can subsequently be set on the CLI or in the config.

This issue is effectively about extending the Jupyter Server CLI. If the modifications are light we can bodge them in, but if we want greater control over the CLI appearance they we would need to create our own argument parser here, parse the args, then reformat argv before calling JupyterServer. Icky but doable.

#!psudopython

def get_option_parser():
    ...
    return parser

@cli_function
def main(..., workflow_id):
    ...
    cylc_opts, jps_opts = parser.parse_known_args()
    if not gui_running or cylc_opts.new:
        argv = ['cylc', 'uiserver', *jps_opts]
        # do Mel's URL magic
        return CylcUIServer.launch_instance(argv)
    else:
        browser.open(get_gui_url())

wxtim · 2022-07-27T07:06:56Z

I may or may not come back to this. Probably not a priority for me.

datamel · 2022-08-08T11:05:38Z

Change of implementation:

Originally we were creating and maintaining our own PID file, to establish if an instance of the gui was running. It turns our Jupyter Server creates two files containing the information that we require. These, by default, are housed in ~/.local/share/jupyter/runtime. The two files created per run are:

jpserver-<pid>.json and
jpserver-<pid>-open.html

The json file contains info such as url, port number, version of jpserver, token etc...
The html file, when called will open the gui at the original page that it was opened at (adding the workflow_id if cylc gui was called with a wofkflow_id argument.)

In this PR, I have changed where this file is created (by setting an environment variable "JUPYTER_RUNTIME_DIR"). Now it can be found in .cylc/uiserver/info_files. Jupyter server cleans these files when shut down correctly but if killed unexpectedly they can remain (I've a bucketload in my dir). I have added a funtion that removes these files if the selected gui process is no longer running - it doesn't loop through all files as I think this could slow down the gui, but if it happens to select one that needs cleaning, it cleans it.

If a gui is started without the --new option it will check for these jp server runtime files and adapt the html file to open at the correct page in the gui.

datamel · 2022-08-11T11:23:58Z

I will update the logic to consider no-browser option.

CHANGES.md

oliver-sanders · 2022-08-17T08:17:12Z

cylc/uiserver/scripts/gui.py

+        if arg.startswith('-'):
+            continue
+        try:
+            loop = asyncio.new_event_loop()


We should only need one loop here. Suggest doing something like this:

def main(*argv): ... = asyncio.run(parse_args()) async def parse_args(): ... for arg in jp_server_opts: ... workflow_id, *_ = await parse_id_async(...)

oliver-sanders · 2022-08-17T08:18:38Z

cylc/uiserver/scripts/gui.py

+            print(
+                f"Workflow '{arg}' does not exist. Opening gui at dashboard.",
+                file=sys.stderr)
+        break


Might as well error here, other Cylc commands would fail with a no such workflow type error (saves the need for the redirect to dashboard thing).

cylc/uiserver/scripts/gui.py

oliver-sanders · 2022-08-17T08:23:11Z

cylc/uiserver/scripts/gui.py

+        pid_match = re.search(r"jpserver-(\d+)-open.html", file)
+        if pid_match is not None:
+            pid = pid_match[1]
+            if pid_exists(int(pid)):


Spanner to throw into the works, the process might not be running on this server.

Does Jupyter Server have a mechanism for checking if a server is still running?

I will have a look and report back, I had forgotten that case.

So, Jupyter Server does have a command for checking if a server is running. I don't think this is of use to us though 😢
The command is jupyter server list

this checks for the existence of pid file (remember this doesn't get cleaned up if the server shuts down unexpectedly)

it then does a os.kill(pid, 0) to check the process is running

it defaults to process running in the case of PermissionError(1, 'Operation not permitted'), which I am meeting regularly as I have a load of stale files.

the result is that it is claiming to have servers running when they are not.

If the process is running on a different server, maybe we can use the info in the pid file to pick the host out?
The info stored in the file is:

{ "base_url": "/", "hostname": "localhost", "password": false, "pid": 11111, "port": 8888, "root_dir": "/net/home/../username", "secure": true, "sock": "", "token": "blah blah some big long token here", "url": "https://localhost:8888/", "version": "1.18.1" }

oliver-sanders

This doesn't seem to detect a gui launched via JupyterHub, likely my fault, can't remember?

oliver-sanders · 2022-08-23T08:48:29Z

cylc/uiserver/app.py

@@ -104,6 +105,9 @@
 from cylc.uiserver.websockets.tornado import TornadoSubscriptionServer
 from cylc.uiserver.workflows_mgr import WorkflowsManager

+RUNTIME_PATH = Path("~/.cylc/uiserver").expanduser()


This is already defined as USER_CONF_ROOT.

cylc/uiserver/scripts/gui.py

datamel · 2022-08-23T12:09:17Z

This doesn't seem to detect a gui launched via JupyterHub, likely my fault, can't remember?

Looking into this now, perhaps due to the env variable maybe.

Update: Discussed at team meeting, needs some thought. Decision made that this PR should go in first and deal with this as a separate issue - raised: #382

oliver-sanders

Tested, all working as expected.

One outstanding comment here - https://github.com/cylc/cylc-uiserver/pull/370/files/cd9796c448fc626d4eb877abb22ef1542d31656a#r952333283

Agree, Jupyter Hub is a different problem (wasn't thinking straight, that needs to be configured at the site-level).

Cheers!

oliver-sanders · 2022-09-29T10:11:50Z

I tested after rebasing this onto master. Suggest maybe rebase-squashing to see the tests pass with latest UIS code.

Remove empty subcommands Change log

MetRonnie

Partial review

cylc/uiserver/scripts/gui.py

cylc/uiserver/app.py

MetRonnie

Aside from my suggestions, I have tested out and happy it's working

MetRonnie · 2022-10-21T08:43:54Z

cylc/uiserver/app.py

    examples = '''
-        cylc gui    # start the cylc GUI
+        cylc gui                  # Start the Cylc GUI (At the dashboard page)
+cylc gui [workflow]       # Start the Cylc GUI (at the workflow page),
+cylc gui --new [workflow] # Start the Cylc GUI (at the workflow page), with a
+                            new instance. By default, if there is an existing
+                            gui instance, Cylc will use that.
+
    '''  # type: ignore[assignment]


"if there is an existing instance, Cylc will use that" applies more to the non --new case, right?

I would also use dedent to tidy this up (from textwrap import dedent):

examples = dedent(''' cylc gui # Start the Cylc GUI (at the dashboard page). cylc gui [workflow] # Start the Cylc GUI (at the workflow page). If there is an existing GUI instance, Cylc will use that. cylc gui --new [workflow] # Start a new instance of the Cylc GUI (at the workflow page). ''') # type: ignore[assignment]

I'll change the wording. The formatting is dealt with through traitlets and they indent and dedent it with yield indent(dedent(self.examples.strip())). An additional dedent as suggested is having no effect for me.

From my testing the dedent is needed to avoid it being like this:

examples = ''' cylc gui # ... cylc gui [workflow] # ...

That is really strange. Two screengrabs from me below with/without dedent:

Incidentally, the If there is an existing GUI instance, Cylc will use that. is required for both the cylc gui and the cylc gui [workflow] options. I actually think it is neater leaving it as it is, since it conveys the same information without repetition, what do you think?

cylc/uiserver/tests/test_gui.py

Add workflow argument to cylc gui command.

Co-authored-by: Oliver Sanders <[email protected]> Add --new option for gui Review Feedback RD

MetRonnie · 2022-12-22T13:55:05Z

cylc/uiserver/scripts/gui.py



 def main(*argv):
    init_log()
-    return CylcUIServer.launch_instance(argv or None)
+    new_gui = CLI_OPT_NEW in argv


This is not used because it is redefined in next line

Suggested change

new_gui = CLI_OPT_NEW in argv

datamel added the small label Jun 23, 2022

datamel added this to the 1.2.0 milestone Jun 23, 2022

datamel self-assigned this Jun 23, 2022

datamel commented Jun 23, 2022

View reviewed changes

datamel mentioned this pull request Jun 27, 2022

2022 Cylc Meetings cylc/cylc-admin#143

Closed

datamel requested a review from oliver-sanders June 28, 2022 13:19

oliver-sanders reviewed Jun 29, 2022

View reviewed changes

cylc/uiserver/scripts/gui.py Outdated Show resolved Hide resolved

cylc/uiserver/scripts/gui.py Outdated Show resolved Hide resolved

cylc/uiserver/scripts/gui.py Outdated Show resolved Hide resolved

hjoliver reviewed Jul 25, 2022

View reviewed changes

cylc/uiserver/app.py Outdated Show resolved Hide resolved

cylc/uiserver/app.py Outdated Show resolved Hide resolved

cylc/uiserver/scripts/gui.py Outdated Show resolved Hide resolved

cylc/uiserver/scripts/gui.py Outdated Show resolved Hide resolved

wxtim self-assigned this Jul 26, 2022

oliver-sanders modified the milestones: 1.3.0, 1.2.0 Jul 28, 2022

datamel force-pushed the add-gui-workflow-arg branch from f0ffeef to 47e8724 Compare August 5, 2022 12:57

datamel requested a review from oliver-sanders August 8, 2022 11:26

oliver-sanders reviewed Aug 17, 2022

View reviewed changes

datamel force-pushed the add-gui-workflow-arg branch 2 times, most recently from bedfb19 to cd9796c Compare August 22, 2022 09:35

oliver-sanders reviewed Aug 23, 2022

View reviewed changes

datamel mentioned this pull request Aug 23, 2022

cylc gui should use the hub (if available) #382

Closed

oliver-sanders approved these changes Sep 29, 2022

View reviewed changes

datamel added 2 commits October 20, 2022 14:57

Add workflow argument to cylc gui command.

467deb0

Update Examples.

8900fc9

datamel added 2 commits October 20, 2022 14:57

Interpret Workflow ID supplied for gui command

61e0be8

Remove empty subcommands Change log

Restrict number of gui instances to one.

d710f28

datamel force-pushed the add-gui-workflow-arg branch from 658c7e6 to 3c882ee Compare October 20, 2022 14:25

MetRonnie reviewed Oct 20, 2022

View reviewed changes

datamel commented Oct 21, 2022

View reviewed changes

cylc/uiserver/app.py Outdated Show resolved Hide resolved

MetRonnie reviewed Oct 21, 2022

View reviewed changes

datamel added 2 commits October 21, 2022 13:54

Add Tests

8566bd4

Add workflow argument to cylc gui command.

Update cylc/uiserver/scripts/gui.py

3806387

Co-authored-by: Oliver Sanders <[email protected]> Add --new option for gui Review Feedback RD

datamel force-pushed the add-gui-workflow-arg branch from 3c882ee to 3806387 Compare October 21, 2022 12:58

tidy

a391c9b

oliver-sanders modified the milestones: 1.2.0, 1.3.0 Dec 7, 2022

MetRonnie reviewed Dec 22, 2022

View reviewed changes

RD review feedback

48b6fff

MetRonnie approved these changes Jan 13, 2023

View reviewed changes

datamel merged commit cb4c843 into cylc:master Jan 18, 2023

datamel deleted the add-gui-workflow-arg branch January 18, 2023 10:09

datamel mentioned this pull request Jan 27, 2023

Make API info available - CLI via Hub/proxy and/or UIS #396

Open

12 tasks

MetRonnie modified the milestones: 1.3.0, 1.2.1 Jan 27, 2023

MetRonnie mentioned this pull request Jan 27, 2023

Fix URL for cylc gui <workflow-id> #415

Merged

7 tasks

datamel mentioned this pull request Mar 1, 2023

Clean gui info files #431

Merged

8 tasks

oliver-sanders mentioned this pull request Sep 20, 2023

cylc gui: detect if a server is already running #240

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add gui workflow arg #370

Add gui workflow arg #370

datamel commented Jun 23, 2022 •

edited

Loading

datamel Jun 23, 2022 •

edited

Loading

codecov-commenter commented Jun 26, 2022 •

edited

Loading

oliver-sanders left a comment

hjoliver left a comment

wxtim commented Jul 26, 2022 •

edited

Loading

hjoliver commented Jul 27, 2022 •

edited

Loading

oliver-sanders commented Jul 27, 2022

wxtim commented Jul 27, 2022

datamel commented Aug 8, 2022

datamel commented Aug 11, 2022

oliver-sanders Aug 17, 2022

oliver-sanders Aug 17, 2022

oliver-sanders Aug 17, 2022

datamel Aug 17, 2022

datamel Aug 17, 2022

oliver-sanders left a comment

oliver-sanders Aug 23, 2022

datamel commented Aug 23, 2022 •

edited

Loading

oliver-sanders left a comment

oliver-sanders commented Sep 29, 2022

MetRonnie left a comment

MetRonnie left a comment

MetRonnie Oct 21, 2022

datamel Oct 21, 2022

MetRonnie Oct 21, 2022

datamel Oct 21, 2022

datamel Oct 21, 2022

MetRonnie Dec 22, 2022

Add gui workflow arg #370

Add gui workflow arg #370

Conversation

datamel commented Jun 23, 2022 • edited Loading

datamel Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Jun 26, 2022 • edited Loading

Codecov Report

oliver-sanders left a comment

Choose a reason for hiding this comment

hjoliver left a comment

Choose a reason for hiding this comment

wxtim commented Jul 26, 2022 • edited Loading

hjoliver commented Jul 27, 2022 • edited Loading

oliver-sanders commented Jul 27, 2022

wxtim commented Jul 27, 2022

datamel commented Aug 8, 2022

datamel commented Aug 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliver-sanders left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datamel commented Aug 23, 2022 • edited Loading

oliver-sanders left a comment

Choose a reason for hiding this comment

oliver-sanders commented Sep 29, 2022

MetRonnie left a comment

Choose a reason for hiding this comment

MetRonnie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datamel commented Jun 23, 2022 •

edited

Loading

datamel Jun 23, 2022 •

edited

Loading

codecov-commenter commented Jun 26, 2022 •

edited

Loading

wxtim commented Jul 26, 2022 •

edited

Loading

hjoliver commented Jul 27, 2022 •

edited

Loading

datamel commented Aug 23, 2022 •

edited

Loading