Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Avoid requiring jupyter_config.py in to be in uiserver subdirectory #594

Draft
wants to merge 1 commit into
base: 1.5.x
Choose a base branch
from
Draft

Avoid requiring jupyter_config.py in to be in uiserver subdirectory #594

wants to merge 1 commit into from

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented May 9, 2024 •
edited
Loading

Closes #565

Previously it was assumed to be in CYLC_SITE_CONF_PATH/uiserver.

Ensure that user is warned if file at CYLC_SITE_CONF_PATH is

  • Unreadable
  • Non-existent

Check List

  • 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).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • CHANGES.md entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@wxtim
Avoid requiring jupyter_config.py in to be in CYLC_SITE_CONF_PATH/uis…
00708ae 
…erver

Remove the need for the uiserver subdirectory.
Ensure that user is warned if file at CYLC_SITE_CONF_PATH is
* Unreadable
* Non-existent
@wxtim wxtim force-pushed the fix.respect_CYLC_SITE_CONF_PATH branch from ac02b14 to 00708ae Compare May 9, 2024 09:15
@wxtim wxtim self-assigned this May 9, 2024
@wxtim wxtim added the small label May 9, 2024
@wxtim wxtim removed the request for review from markgrahamdawson May 9, 2024 09:16
@wxtim wxtim added this to the 1.4.5 milestone May 9, 2024
@wxtim wxtim linked an issue May 31, 2024 that may be closed by this pull request
UI Server Configuration does not seem to honour $CYLC_SITE_CONF_PATH environment variable #565
Open
@MetRonnie MetRonnie modified the milestones: 1.4.5, 1.5.1 Jun 18, 2024
@MetRonnie MetRonnie changed the base branch from 1.4.x to 1.5.x June 19, 2024 12:43
@oliver-sanders oliver-sanders modified the milestones: 1.5.1, 1.6.0 Oct 15, 2024
oliver-sanders
oliver-sanders requested changes Oct 15, 2024
View reviewed changes
Copy link
Member

@oliver-sanders oliver-sanders left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously it was assumed to be in CYLC_SITE_CONF_PATH/uiserver.

This was correct and consistent with cylc-flow which looks in $CYLC_SITE_CONF_PATH/flow.

The CYLC_SITE_CONF_PATH directory is supposed to look like this:

.
|-- flow
|   `-- global.cylc
`-- uiserver
    `-- jupyter_config.py

The intention is that it can be used to configure all of the Cylc components.

As of the current versions setting CYLC_SITE_CONF_PATH to the root of the above structure works fine for me.

@wxtim
Copy link
Member Author

wxtim commented Oct 15, 2024

As of the current versions setting CYLC_SITE_CONF_PATH to the root of the above structure works fine for me.

So this can be closed?

@oliver-sanders
Copy link
Member

Check the issue.

@wxtim wxtim marked this pull request as draft October 16, 2024 09:11
@oliver-sanders
Copy link
Member

oliver-sanders commented Oct 17, 2024 •
edited
Loading

Had a read through the issue, I think these are the main points:

  1. The config file didn't get picked up in the location the docs suggested.
    • Check the current behaviour (i.e. $CYLC_SITE_CONF_DIR/uiserver/jupyter_config.py) makes sense and is consistent with cylc-flow.
    • If so, amend the docs as needed.
  2. If the config file is not readable, there is no error.
    • This is likely Jupyter Server behaviour (try reproducing this using jupyter server).
    • If this is special Cylc behaviour, not Jupyter Server behaviour, then it's a bug.
    • Otherwise, we could consider pushing an upstream change to Jupyter Server.
  3. If no config files are found / readable, they would like a warning stating that users have no access.
    • I'm not sure I get this bit.

@wxtim
Copy link
Member Author

wxtim commented Oct 23, 2024

Had a read through the issue, I think these are the main points:

Sorry, so have I, just not been high enough on my todo-list. It is on my radar.

@wxtim
Copy link
Member Author

wxtim commented Oct 24, 2024 •
edited
Loading

The config file didn't get picked up in the location the docs suggested.

This works as documented here, although there might be a case for an extra note on the uiserver app docs.

@wxtim
Copy link
Member Author

wxtim commented Oct 24, 2024

@oliver-sanders - I'm not convinced that this PR is necessary.

@hjoliver
Copy link
Member

hjoliver commented Nov 7, 2024

Can we close this then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliver-sanders oliver-sanders oliver-sanders requested changes

Assignees

@wxtim wxtim

Labels
small
Projects
None yet
Milestone
1.6.0
Development

Successfully merging this pull request may close these issues.

UI Server Configuration does not seem to honour $CYLC_SITE_CONF_PATH environment variable
4 participants
@wxtim @oliver-sanders @hjoliver @MetRonnie