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.

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

purge filetree, clean up a few misc items #108

Merged
merged 3 commits into from
Jun 3, 2021
Merged

purge filetree, clean up a few misc items #108

merged 3 commits into from
Jun 3, 2021

Conversation

timkpaine
Copy link
Collaborator

  • Purging filetree from the code base, this should allow @martinRenou to PR his file tree fork upstream
  • A few misc items in readme, makefile
  • Add contents manager override during install (@telamonian should we do this automatically?)

@github-actions
Copy link

Binder Launch a binder notebook on branch timkpaine/jupyter-fs/tkp/clean

@martinRenou
Copy link

I see that the jupyterlab-filetree code was copied there, would you like a PR to jupyter-fs directly?

Do you think we should copy the code or should we install the federated extension with pip/conda?

@telamonian
Copy link
Contributor

* Add contents manager override during install (@telamonian should we do this automatically?)

damn right we should. Very sadly just adding the new ContentsManager class directly to jupyterfs.json doesn't work! I think we'd need @Zsailer to chime in and explain why this situation exists/where to start on a fix.

Other options:

  • Manually determine ${PREFIX}/etc/jupyter/jupyter_server_config.json path using sysconfig (or possibly the machinery behind jupyter --config), then manually merge "contents_manager_class": "jupyterfs.metamanager.MetaManager" setting into any existing jupyter_server_config.json

    • Cons:
      • convoluted
      • fragile
      • what to do on conflict
  • Ignore jupyter config system entirely, use a filthy hack (a la the ever popular jupytext) to set ContentsManager instance in our own code

    • Cons:
      • Will cause undefined (destructive?) behavior the instant the user installs two extensions that try to use the same trick (eg this extension + jupyterfs)

Anyway, the last time I thought about this I essentially threw up my hands and decided it was a documentation problem. The options are not great

Copy link
Contributor

@telamonian telamonian left a comment

Choose a reason for hiding this comment

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

LGTM! I rebased this onto lastest main and also got rid of the change to the config file

@telamonian telamonian merged commit 2130baf into jpmorganchase:main Jun 3, 2021
@telamonian
Copy link
Contributor

telamonian commented Jun 3, 2021

Do you think we should copy the code or should we install the federated extension with pip/conda?

@martinRenou I think there's 4 practical options to deal with this:

  1. Get two separate extension pkgs to play nicely with each other, maybe following something like the provider/consumber pattern
  2. Refactor the dependee into separate lib and extension pkgs, making sure to move all exports to the lib pkg
  3. Vend a copy of the dependee codebase into the dependent pkg's repo
  4. Structure all inter-extension pkg interactions in terms of a set of lumino command items

Option 2) is probably the best (at least in terms of what problems it can cause), and in any case it may make good engineering sense to structure your project as separate lib and extension pkgs. If it doesn't make sense to split up your project that way, then 3) is the better fallback; vended code tends to cause heartbreak the moment it gets out of sync with its HEAD.

@timkpaine timkpaine deleted the tkp/clean branch December 3, 2022 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants