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

Fix: Remove default jupyter state vars that triggers requests to unconfigured server on 0.17.x (#338) #341

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

MarcosVn
Copy link
Contributor

Fixes one scenario reported in #338.

@MarcosVn MarcosVn changed the title Fix: Removing default jupyter vars that triggers requests to unconfigured server on 0.17.x (#338) Fix: Remove default jupyter state that triggers requests to unconfigured server on 0.17.x (#338) Dec 18, 2024
@MarcosVn MarcosVn changed the title Fix: Remove default jupyter state that triggers requests to unconfigured server on 0.17.x (#338) Fix: Remove default jupyter state vars that triggers requests to unconfigured server on 0.17.x (#338) Dec 18, 2024
@echarles
Copy link
Member

Thank you @MarcosVn. JupyterLab considers you need a server, so the bootstrap part has always been tricky.

Could you double check in various scenario (server, nbmodel without server, bringing the servicemanager or the kernel, notebook, cell, terminal, jupyterlabapp, jupyter context, jupyterreacttheme) and confirm there is no negative impact before I merge (just use the examples in webpackconfig).

Thank you again!

@MarcosVn
Copy link
Contributor Author

Hey @echarles, I tested the following components:

  • ./src/app/App
  • ./src/examples/Cell
  • ./src/examples/Cells
  • ./src/examples/CellLite
  • ./src/examples/Console
  • ./src/examples/ConsoleLite
  • ./src/examples/Deno
  • ./src/examples/JupyterLabApp
  • ./src/examples/JupyterLabHeadlessApp
  • ./src/examples/Kernels
  • ./src/examples/Notebook
  • ./src/examples/NotebookKernelChange
  • ./src/examples/NotebookLite
  • ./src/examples/NotebookNbFormat
  • ./src/examples/NotebookTheme
  • ./src/examples/NotebookThemeColorMode
  • ./src/examples/Output
  • ./src/examples/Outputs
  • ./src/examples/Terminal
  • ./src/examples/Viewer

All of them worked well without any apparent inconsistencies caused by the fix for this issue.

However, some of them showed problems that occur even in the HEAD of version 0.17.x, meaning they are unrelated to the changes in this issue.

Some cases include:

  • ./src/examples/Deno -> (File Load Error for 10.2_Polar DataFrames.ipynb);
  • ./src/examples/JupyterLabApp -> Did not load the ipynb files correctly for use;
  • ./src/examples/JupyterLabHeadlessApp -> Did not load the ipynb files correctly for use;
  • ./src/examples/NotebookLite -> Could not find "ping.ipynb".

Since these issues occur even outside of this PR, I believe we can ignore them, right?

One more thing I noticed is that in the 0.17.x branch, we ended up bringing in the Kernel state for Cell and Output (#282) evolutions, which I personally hadn't used or tested until now. Is there anything about these changes that I should keep in mind or be concerned about? (such as any subsequent fixes)

Copy link
Member

@echarles echarles left a comment

Choose a reason for hiding this comment

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

LGTM Thx @MarcosVn

@echarles
Copy link
Member

Great. I have opened #342 (will look at it asap).

Merging this PR. Thx again @MarcosVn

@echarles echarles merged commit f8f6ce7 into datalayer:0.17.x Dec 19, 2024
3 of 4 checks passed
@MarcosVn
Copy link
Contributor Author

MarcosVn commented Dec 19, 2024

Hey, @echarles, would it be possible for you to publish version 0.17.2 with this fix? Thanks in advance as well!

@echarles
Copy link
Member

Just published 0.17.2 https://www.npmjs.com/package/@datalayer/jupyter-react/v/0.17.2

Curious, any reason to stick to 0.17.x and not migrate to latest?

@MarcosVn
Copy link
Contributor Author

MarcosVn commented Dec 19, 2024

Just published 0.17.2 https://www.npmjs.com/package/@datalayer/jupyter-react/v/0.17.2

Thank you, @echarles

Curious, any reason to stick to 0.17.x and not migrate to latest?

The reason was mainly due to the fact that we extensively use the cell-related features in our component, and the evolution: "Kernel state for Cell and Output (#282)" or other similar evolution (I really don't remember 100%, sorry) was introduced after we had already tested and developed based on a previous version.

In other words, the idea was to better understand what changed in #282 before using the latest version to avoid risks in our app.

  • Note: I briefly tried using a more recent version (0.18.x>) here as well and experienced some issues with the server connection. I can try again and report exactly what happened if it still occurs.

That said, I noticed that even our 0.17.x branch ended up integrating #282, so my initial argument is no longer valid. However, in our tests, version 0.17.x seems quite stable.

Taking advantage of the context, @echarles: Were there any relevant adjustments or changes made after #282 that I should be concerned about?

@echarles
Copy link
Member

Thx for sharing context.

Taking advantage of the context, @echarles: Were there any relevant adjustments or changes made after #282 that I should be concerned about?

It is hard to tell without knowledge of your case and usage. This said, the big changes are before and are moving from Redux to Zustand, and add a useJupyter on top of the Jupyter React Context.

At some point staying on 0.17 will limit you for the new big features, happy to help resolving any issue you would report with latest main.

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.

2 participants