Adds version hierarchy to config files #301

datamel · 2022-01-28T13:56:13Z

For hub_version 0.6.0, the load order can be seen below:

These changes close #199 and #291

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).
Appropriate tests are included (unit and/or functional).
Appropriate change log entry included.
I have opened a documentation PR at cylc/cylc-doc/pull/XXXX.
No dependency changes.

codecov-commenter · 2022-01-28T15:24:51Z

Codecov Report

Merging #301 (3dade03) into master (9ca3cad) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #301      +/-   ##
==========================================
- Coverage   79.61%   79.57%   -0.05%     
==========================================
  Files           9        9              
  Lines         981      979       -2     
  Branches      193      193              
==========================================
- Hits          781      779       -2     
  Misses        165      165              
  Partials       35       35

Impacted Files	Coverage Δ
cylc/uiserver/app.py	`85.96% <100.00%> (-0.25%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ca3cad...3dade03. Read the comment docs.

MetRonnie

Should these two be deleted?

cylc-uiserver/cylc/uiserver/jupyterhub_config.py

Lines 45 to 48 in 0c17a39

    
           # site configuration 
        
           SITE_CONF_PATH: Path = SITE_CONF_ROOT / 'jupyter_config.py' 
        
           # user configuration 
        
           USER_CONF_PATH: Path = USER_CONF_ROOT / 'jupyter_config.py'

cylc/uiserver/app.py

MetRonnie · 2022-01-31T10:19:26Z

cylc/uiserver/jupyterhub_config.py

@@ -49,21 +55,35 @@ def _load(path):
        exec(path.read_text())


+def get_conf_dir_hierarchy(config_paths: List[Path]):


Suggested change

def get_conf_dir_hierarchy(config_paths: List[Path]):

def get_conf_dir_hierarchy(config_paths: List[Path]) -> List[Path]:

MetRonnie · 2022-01-31T10:20:23Z

cylc/uiserver/jupyterhub_config.py

+        conf_hierarchy.append(Path(x[0], x[1], 'jupyter_config.py'))
+    return conf_hierarchy
+
+
 def load():


(Type annotations in a function body aren't checked unless the definition has type annotations)

Suggested change

def load():

def load() -> None:

MetRonnie · 2022-01-31T10:20:36Z

cylc/uiserver/jupyterhub_config.py

        )
    else:
-        site_conf_path: Path = SITE_CONF_PATH
-    config_paths = [DEFAULT_CONF_PATH, site_conf_path, USER_CONF_PATH]
+        site_conf_path: Path = SITE_CONF_ROOT


Already declared

Suggested change

site_conf_path: Path = SITE_CONF_ROOT

site_conf_path = SITE_CONF_ROOT

MetRonnie · 2022-01-31T11:01:39Z

For some reason it's not using the jupyter_config.py I have moved from ~/.cylc/hub/ to ~/.cylc/uiserver/0.6/. I have verified that it gets loaded using breakpoints, but the UI build being used is the bundled one instead of the one specified

c.CylcUIServer.ui_build_dir = '<home>/github/cylc-ui/dist'

datamel · 2022-01-31T11:04:29Z

For some reason it's not using the jupyter_config.py I have moved from ~/.cylc/hub/ to ~/.cylc/uiserver/0.6/. I have verified that it gets loaded using breakpoints, but the UI build being used is the bundled one instead of the one specified
c.CylcUIServer.ui_build_dir = '<home>/github/cylc-ui/dist'

I'll look into this now, thanks @MetRonnie

MetRonnie · 2022-01-31T13:25:05Z

Here is the callstack for that breakpoint, in case it's any use

oliver-sanders · 2022-01-31T14:48:39Z

I have verified that it gets loaded using breakpoints

It could be loading other config files which reset it back, try running in debug mode, you should see a log entry every time something changed.

MetRonnie · 2022-01-31T17:26:08Z

cylc hub --debug

I have found no real difference in the log output with this PR or master checked out, other than:

the expected ~/.cylc/hub -> ~/.cylc/uiserver

-[I CylcUIServer] Serving UI from: ~/github/cylc-ui/dist
+[I CylcUIServer] Serving UI from: ~/github/cylc-uiserver/cylc/uiserver/ui/0.6.0

In neither was there an indication of what jupyter_config.py was loaded.

MetRonnie · 2022-01-31T17:36:23Z

(Updated)

This bit of logging only works for cylc gui --debug, not cylc hub --debug:

cylc-uiserver/cylc/uiserver/app.py

Lines 379 to 384 in aa31dea

    
           self.log.debug( 
        
               'CylcUIServer config:\n' + '\n'.join( 
        
                   f'  * {key} = {repr(value)}' 
        
                   for key, value in self.config['CylcUIServer'].items() 
        
               ) 
        
           )

This doesn't work for either:

cylc-uiserver/cylc/uiserver/jupyterhub_config.py

Line 48 in 5ce8fa6

LOG.info(f'Loading config file: {path}')

This is also the case on master

oliver-sanders · 2022-01-31T17:36:42Z

cylc gui --debug

oliver-sanders

Should do the trick for JupyterHub, needs a small addition to make it work for the UI Server too.

oliver-sanders · 2022-01-31T18:07:28Z

cylc/uiserver/jupyterhub_config.py

@@ -49,21 +55,35 @@ def _load(path):
        exec(path.read_text())


+def get_conf_dir_hierarchy(config_paths: List[Path]):


This file is only loaded by JupyterHub, to open it up to Jupyter Server too need to add it to the list of config_file_paths in cylc.uiserver.CylcUIServer

Need to update?

cylc-uiserver/cylc/uiserver/app.py

Lines 103 to 115 in 1b2a599

config_file_paths = list(

map(

str,

[

# user configuration

USER_CONF_ROOT,

# site configuration

SITE_CONF_ROOT,

# base configuration - always used

Path(uis_pkg).parent,

]

)

)

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

oliver-sanders · 2022-02-01T15:23:59Z

cylc/uiserver/app.py

        )
    )
+    config_file_paths.append(str(Path(uis_pkg).parent))


We should add the path hierarchy here rather than loading the hub config, this lets Jupyter Server take care of loading the files correctly.

MetRonnie

hub -> uiserver here?

cylc-uiserver/cylc/uiserver/app.py

Lines 20 to 22 in 3dade03

    
           * ``$CYLC_SITE_CONF_PATH/hub/jupyter_config.py`` (for site-level configuration) 
        
           * ``/etc/cylc/hub/jupyter_config.py`` (for system-level configuration) 
        
           * ``~/.cylc/hub/jupyter_config.py`` (for user-level configuration)

MetRonnie

It is correctly loading the moved jupyter_config.py now

cylc/uiserver/jupyterhub_config.py

MetRonnie

Ah wait, forgot about the requested change in my earlier comment

hub -> uiserver here?

cylc-uiserver/cylc/uiserver/app.py

Lines 20 to 22 in 3dade03

* ``$CYLC_SITE_CONF_PATH/hub/jupyter_config.py`` (for site-level configuration)

* ``/etc/cylc/hub/jupyter_config.py`` (for system-level configuration)

* ``~/.cylc/hub/jupyter_config.py`` (for user-level configuration)

datamel · 2022-02-02T09:11:13Z

Ah wait, forgot about the requested change in my earlier comment

hub -> uiserver here?

cylc-uiserver/cylc/uiserver/app.py

Lines 20 to 22 in 3dade03

* ``$CYLC_SITE_CONF_PATH/hub/jupyter_config.py`` (for site-level configuration)

* ``/etc/cylc/hub/jupyter_config.py`` (for system-level configuration)

* ``~/.cylc/hub/jupyter_config.py`` (for user-level configuration)

Great, well spotted, that just came in on a rebase.

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

…nto config-root-path

oliver-sanders · 2022-02-02T16:07:35Z

(flake8 unhappy)

oliver-sanders

Sorry, could do with shifting the logic out of the hub config, this file is only intended for JupyterHub and is not supposed to be imported directly for normal use because it auto-loads.

cylc/uiserver/app.py

cylc/uiserver/jupyterhub_config.py

oliver-sanders

Looks good to me, tested as working, just need to get the test passing.

cylc/uiserver/app.py

Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>

datamel · 2022-02-03T13:14:32Z

Looks good to me, tested as working, just need to get the test passing.

Yep thanks! On it now. I can't reproduce locally but I expect a path expansion problem.

cylc/uiserver/tests/test_config.py

Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>

datamel self-assigned this Jan 28, 2022

datamel requested review from oliver-sanders and MetRonnie January 28, 2022 14:51

datamel added this to the cylc-uiserver 1.0.0 milestone Jan 28, 2022

MetRonnie reviewed Jan 31, 2022

View reviewed changes

oliver-sanders reviewed Jan 31, 2022

View reviewed changes

datamel added 4 commits February 1, 2022 13:28

Adds version hierarchy to config files

c43a875

change log

47e909d

mypy

30961a4

Add hierachy to config_file_paths

efcaef1

datamel force-pushed the config-root-path branch from 0e69c4e to efcaef1 Compare February 1, 2022 13:43

datamel and others added 2 commits February 1, 2022 13:44

Update cylc/uiserver/app.py

0353646

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

Fix circular import

3dade03

oliver-sanders reviewed Feb 1, 2022

View reviewed changes

MetRonnie reviewed Feb 1, 2022

View reviewed changes

MetRonnie approved these changes Feb 1, 2022

View reviewed changes

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

MetRonnie requested changes Feb 1, 2022

View reviewed changes

datamel and others added 3 commits February 2, 2022 09:11

Update cylc/uiserver/jupyterhub_config.py

a9f0f2f

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

Add documentation

2ab72c5

Merge branch 'config-root-path' of github.com:datamel/cylc-uiserver i…

d3254f8

…nto config-root-path

oliver-sanders reviewed Feb 2, 2022

View reviewed changes

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

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

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

datamel added 5 commits February 3, 2022 12:01

Review Feedback

a97ddd3

Fix documentation of config hierarchy

ff9c8c1

mypy

229507e

flake8

b871285

test fix for GH Actions

016093a

oliver-sanders approved these changes Feb 3, 2022

View reviewed changes

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

Update cylc/uiserver/app.py

0ba4fe1

Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>

oliver-sanders reviewed Feb 3, 2022

View reviewed changes

cylc/uiserver/tests/test_config.py Outdated Show resolved Hide resolved

Update cylc/uiserver/tests/test_config.py

9afa790

Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>

oliver-sanders requested a review from MetRonnie February 3, 2022 15:12

MetRonnie approved these changes Feb 3, 2022

View reviewed changes

oliver-sanders linked an issue Feb 3, 2022 that may be closed by this pull request

configuration path root #291

Closed

oliver-sanders merged commit 6029995 into cylc:master Feb 3, 2022

datamel mentioned this pull request Feb 10, 2022

Fix log level for config loading #307

Closed

7 tasks

This was referenced Mar 4, 2022

README update of UIServer config path #326

Merged

ui config #232

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds version hierarchy to config files #301

Adds version hierarchy to config files #301

datamel commented Jan 28, 2022 •

edited

Loading

codecov-commenter commented Jan 28, 2022 •

edited

Loading

MetRonnie left a comment

MetRonnie Jan 31, 2022

MetRonnie Jan 31, 2022

MetRonnie Jan 31, 2022

MetRonnie commented Jan 31, 2022

datamel commented Jan 31, 2022

MetRonnie commented Jan 31, 2022

oliver-sanders commented Jan 31, 2022

MetRonnie commented Jan 31, 2022

MetRonnie commented Jan 31, 2022 •

edited

Loading

oliver-sanders commented Jan 31, 2022

oliver-sanders left a comment

oliver-sanders Jan 31, 2022

MetRonnie Jan 31, 2022

oliver-sanders Feb 1, 2022

MetRonnie left a comment

MetRonnie left a comment

MetRonnie left a comment

datamel commented Feb 2, 2022

oliver-sanders commented Feb 2, 2022

oliver-sanders left a comment

oliver-sanders left a comment

datamel commented Feb 3, 2022

	# site configuration
	SITE_CONF_PATH: Path = SITE_CONF_ROOT / 'jupyter_config.py'
	# user configuration
	USER_CONF_PATH: Path = USER_CONF_ROOT / 'jupyter_config.py'

		@@ -49,21 +55,35 @@ def _load(path):
		exec(path.read_text())


		def get_conf_dir_hierarchy(config_paths: List[Path]):

	site_conf_path: Path = SITE_CONF_ROOT
	site_conf_path = SITE_CONF_ROOT

	config_file_paths = list(
	map(
	str,
	[
	# user configuration
	USER_CONF_ROOT,
	# site configuration
	SITE_CONF_ROOT,
	# base configuration - always used
	Path(uis_pkg).parent,
	]
	)
	)

	* ``$CYLC_SITE_CONF_PATH/hub/jupyter_config.py`` (for site-level configuration)
	* ``/etc/cylc/hub/jupyter_config.py`` (for system-level configuration)
	* ``~/.cylc/hub/jupyter_config.py`` (for user-level configuration)

Adds version hierarchy to config files #301

Adds version hierarchy to config files #301

Conversation

datamel commented Jan 28, 2022 • edited Loading

codecov-commenter commented Jan 28, 2022 • edited Loading

Codecov Report

MetRonnie left a comment

Choose a reason for hiding this comment

MetRonnie Jan 31, 2022

Choose a reason for hiding this comment

MetRonnie Jan 31, 2022

Choose a reason for hiding this comment

MetRonnie Jan 31, 2022

Choose a reason for hiding this comment

MetRonnie commented Jan 31, 2022

datamel commented Jan 31, 2022

MetRonnie commented Jan 31, 2022

oliver-sanders commented Jan 31, 2022

MetRonnie commented Jan 31, 2022

MetRonnie commented Jan 31, 2022 • edited Loading

oliver-sanders commented Jan 31, 2022

oliver-sanders left a comment

Choose a reason for hiding this comment

oliver-sanders Jan 31, 2022

Choose a reason for hiding this comment

MetRonnie Jan 31, 2022

Choose a reason for hiding this comment

oliver-sanders Feb 1, 2022

Choose a reason for hiding this comment

MetRonnie left a comment

Choose a reason for hiding this comment

MetRonnie left a comment

Choose a reason for hiding this comment

MetRonnie left a comment

Choose a reason for hiding this comment

datamel commented Feb 2, 2022

oliver-sanders commented Feb 2, 2022

oliver-sanders left a comment

Choose a reason for hiding this comment

oliver-sanders left a comment

Choose a reason for hiding this comment

datamel commented Feb 3, 2022

datamel commented Jan 28, 2022 •

edited

Loading

codecov-commenter commented Jan 28, 2022 •

edited

Loading

MetRonnie commented Jan 31, 2022 •

edited

Loading