♻️ Make ipylab an optional dependency #282
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #282 +/- ##
==========================================
+ Coverage 89.22% 89.35% +0.13%
==========================================
Files 24 24
Lines 1039 1033 -6
==========================================
- Hits 927 923 -4
+ Misses 112 110 -2 ☔ View full report in Codecov by Sentry. |
|
|
||
| # trying to init ipylab JupyterFrontEnd can lead to errors on jupyter notebook | ||
| try: | ||
| _init_frontend() |
There was a problem hiding this comment.
I don't agree with this, this is here for a good reason and definitely should be called if ipylab is installed. I would prefer to revert this, also it is in try catch block, so no problems if ipylab is not installed.
There was a problem hiding this comment.
I don't very much mind if this is being reverted if it's not preventing to eliminate ipylab:
I removed it because I thought it may cause latency due to the error.
Alternatively, we port everything we need from nbproject into lamindb and no longer rely on nbproject.
But to be honest, I don't think anybody uses these interactive features, so, I don't think it's worth investing anytime that's not meant to streamline lamindb (make lamindb higher quality, faster, more lightweight, etc.).
There was a problem hiding this comment.
The problem is that ipylab is used not only for interactive features, but also for detecting the notebook path if everything else fails, this is relevant for some cloud services and we should add a warning in such cases.
There was a problem hiding this comment.
Yes, I saw this! If it's hitting it's now going to error with ipylab isn't installed. And if this is actually occurring somewhere, we'll then learn about it.
The logic is still there! I just made ipylab optional.
There was a problem hiding this comment.
But it should not error in this case, it should till try the environment variable. Ok, i will make a PR.
There was a problem hiding this comment.
Yes, please, just make a PR that restores what you think should be there.
I also think it's perfectly fine if nbproject has an ipylab extra, but I'd prefer to not install it through lamindb[jupyter].
|
Can we pause this until i analyse how to disentangle this properly? |
|
We should have an extra for ipylab and a warning somewhere at least. But i don't think it makes sense at all to have nbproject without ipylab. |
I hope we get away without ipylab. The |
No description provided.