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

JupyterLab 3 update #405

Merged
merged 13 commits into from
Jan 6, 2021
Merged

JupyterLab 3 update #405

merged 13 commits into from
Jan 6, 2021

Conversation

martinRenou
Copy link
Member

@martinRenou martinRenou commented Nov 9, 2020

This is the quick path to get this working with JupyterLab 3, not finished yet. I still need to make jupyterlab-lsp a federated labextension.

@martinRenou martinRenou mentioned this pull request Nov 9, 2020
7 tasks
@@ -2,6 +2,7 @@
.mypy_cache
.pytest_cache
.yarn-packages
**/_*.d.ts
Copy link
Collaborator

Choose a reason for hiding this comment

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

if these end up in the dist (which they might) we will want them formatted...

requirements/lint.yml Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
Signed-off-by: martinRenou <martin.renou@gmail.com>
Signed-off-by: martinRenou <martin.renou@gmail.com>
binder/postBuild Outdated
# actually build
jupyter lab build --debug --dev-build=False --minimize=True
# Do a dev install of the server side
pip install . -vv
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please bring back python -m pip install -e . --ignore-installed --no-deps -vv... it's not too livehackable, today (vs #278), but would like to maintain that experience.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I'll bring it back

Copy link
Member Author

Choose a reason for hiding this comment

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

Although -e will not install the data_files, but we need them installed

setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
Signed-off-by: martinRenou <martin.renou@gmail.com>

chdir('../../../')

extension_files.append(("etc/jupyter/jupyter_notebook_config.d", ["py_src/jupyter_lsp/etc/jupyter-lsp-serverextension.json"]))
Copy link
Collaborator

Choose a reason for hiding this comment

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

as has been mentioned elsewhere: maybe we have the structure in-package already looks like the system-of-interest, e.g. etc/ and share/ so the patterns are more concise.

Signed-off-by: martinRenou <martin.renou@gmail.com>
@@ -118,6 +135,7 @@
]
}
},
"schemaDir": "schema"
"schemaDir": "schema",
"outputDir": "../../py_src/jupyter_lsp/labextensions/@krassowski/jupyterlab-lsp"
Copy link
Collaborator

Choose a reason for hiding this comment

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

we need to add checking all of these (and probably other things) to integrity.py

Signed-off-by: martinRenou <martin.renou@gmail.com>
requirements/lint.yml Outdated Show resolved Hide resolved
setup.cfg Outdated
@@ -1,5 +1,5 @@
[metadata]
name = jupyter-lsp
name = jupyter_lsp
Copy link
Collaborator

Choose a reason for hiding this comment

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

wut

Copy link
Member Author

Choose a reason for hiding this comment

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

This makes it importable :) enabling jupyter labextension develop

Copy link
Collaborator

Choose a reason for hiding this comment

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

distributed package name != import name... this is explicitly different because of that. like i mentioned elsewhere, if something in the stack is conflating the two, it needs to be nuked from orbit. If it has to be like this, then that amplifies the need for it to be a standalone package.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am totally fine reverting this change, just note that you won't be able to use jupyter labextension develop.

then that amplifies the need for it to be a standalone package

Yes, although I wanted to keep this PR simple for now and not make another package.

Choose a reason for hiding this comment

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

Signed-off-by: martinRenou <martin.renou@gmail.com>
Signed-off-by: martinRenou <martin.renou@gmail.com>
@bollwyvl
Copy link
Collaborator

Pretty sure we discussed this, but think we might want to have a dependency tree like:

jupyterlab_lsp ---+---> jupyter_lsp ----+---> jupyter_server
                  \---> jupyterlab ----/

Motivation: we have been trying really hard to make the contract between the frontend and backend packages "regular" (e.g. schema-constrained). If the frontend starts getting shipped with the backend, this would become less formal, and downstreams that wish to extend the functionality would be at a disadvantage, and more likely to fork.

Further, if we split out and version individual features in the future, jupyterlab_lsp can become a dependency-only metapackage, we can add 100 packages, and jupyter_lsp (but more importanty, docs) wouldn't change at all.

We'll do a major version bump for jupyter_lsp, and a can copy that version for jupyterlab_lsp, and they'll have pretty hard pins between the two of them. If anything, I've considered having a third package which only held "our" JSON schema version of the LSP and the definition of the extra metadata stuff we need... but that's for another day.

Signed-off-by: martinRenou <martin.renou@gmail.com>
Signed-off-by: martinRenou <martin.renou@gmail.com>
Signed-off-by: martinRenou <martin.renou@gmail.com>
@martinRenou
Copy link
Member Author

Pretty sure we discussed this, but think we might want to have a dependency tree like

Not sure I understand the tree structure you are drawing here.

But I guess you are mentioning to make a separate Python package for the labextension? It might be indeed nicer. Although I didn't want this PR to drastically change the code structure.

The PR works now. You should get it to work with:

jlpm bootstrap
pip install .
jupyter serverextension enable --sys-prefix --py jupyter_lsp

Please don't hesitate to push to my branch if you want to change/polish some things.

@bollwyvl
Copy link
Collaborator

bollwyvl commented Nov 13, 2020 via email

@martinRenou
Copy link
Member Author

Would running "labextension develop" do the thing?

Yes, it should. labextension develop will make a symlink from the local build to the share/jupyter/labextension directory. But we would need to figure out the package name issue (jupyter-lsp vs jupyter_lsp). This is something that should be fixed in JupyterLab itself maybe, it tries to import the package to find the _jupyter_labextension_paths function. Maybe JupyterLab could use importlib.importmodule('jupyter-lsp') so that it works.

@bollwyvl
Copy link
Collaborator

bollwyvl commented Nov 13, 2020 via email

@karlaspuldaro
Copy link
Contributor

Nice work! Looking forward to seeing this in lab3 :)
A couple suggestions so far.

As per comments in #411 maybe this PR can update the docs to use node 12+ required to dev for lab3.
And I saw rc10 is out, that could also be tested out and updated here.

If the suggestions are valid, let me know if you need any help then I can create a PR for your branch :)

@martinRenou
Copy link
Member Author

Thanks for the suggestions. Yes, don't hesitate to open a PR to my branch :)

@bollwyvl
Copy link
Collaborator

This seems close to coming home... @krassowski et al: how do we want to handle landing this? Two options:

  • create a 3.x, and land this PR against it
  • create a 2.x, and land this against master

As there is still no firm release date for lab 3, i'm favoring the former, but it's kinda all the same 🤷

Also as this is going to be turbulent, we might want to investigate looking into the more complicated RTD setup which gives us versioned releases.

@martinRenou
Copy link
Member Author

As pointed out by @karlaspuldaro only the readthedocs build is triggered on the CI. Any idea why Github actions is not triggered here? Maybe it's because of the draft status, I will change this

@martinRenou martinRenou marked this pull request as ready for review November 26, 2020 16:05
@martinRenou
Copy link
Member Author

I will need to rebase and resolve conflicts

@krassowski
Copy link
Member

Yes, I think it was the draft status, but if CI does not trigger with the next push let me know and I will investigate further. I will look into branching/catching up with releases later on Wednesday.

@krassowski krassowski merged commit c90dac3 into jupyter-lsp:master Jan 6, 2021
@martinRenou martinRenou deleted the jupyterlab3 branch January 6, 2021 16:50
@martinRenou
Copy link
Member Author

🎉 Thanks a lot!

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.

6 participants