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

Remove Yjs room locks #12288

Merged
merged 2 commits into from Mar 30, 2022
Merged

Remove Yjs room locks #12288

merged 2 commits into from Mar 30, 2022

Conversation

davidbrochart
Copy link
Contributor

Code changes

Remove the Yjs room locks.

User-facing changes

None

Backwards-incompatible changes

Rename WebSocketProviderWithLocks to WebSocketProvider in @jupyterlab/docprovider.

@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@jtpio
Copy link
Member

jtpio commented Mar 29, 2022

Thanks @davidbrochart.

Mind providing some more context on what removing the locks will mean in practice?

Or is it a follow-up to #11599, which means they are not needed anymore?

@jtpio jtpio added this to the 4.0 milestone Mar 29, 2022
@davidbrochart
Copy link
Contributor Author

Or is it a follow-up to #11599, which means they are not needed anymore?

Sorry for not providing more context, yes it is. Because it is not backwards-compatible, we decided not to include it in #11599, so that it can be back-ported. But yes, the locks are not needed anymore, now that we save from the back-end.

@jtpio jtpio added the api-change A change that should be accompanied by a major version increase label Mar 29, 2022
@jtpio
Copy link
Member

jtpio commented Mar 29, 2022

OK thanks.

Rename WebSocketProviderWithLocks to WebSocketProvider in @jupyterlab/docprovider.

Let's document this in the extension migration guide? We can also mention the removal of acquireLock and releaseLock from the interface.

https://jupyterlab.readthedocs.io/en/latest/extension/extension_migration.html#api-breaking-changes

@davidbrochart
Copy link
Contributor Author

It looks like the Linux JS Flaky Tests failures are unrelated, as well as the pre-commit ones.

@afshin afshin closed this Mar 30, 2022
@afshin afshin reopened this Mar 30, 2022
@afshin
Copy link
Member

afshin commented Mar 30, 2022

Is it possible to pass this schema as a local file instead of requiring an outbound HTTP request in the pre-commit workflow?

args: ['--schemafile', 'https://json.schemastore.org/github-workflow']

Is that what is breaking here?

pre-commit ci failure

@afshin
Copy link
Member

afshin commented Mar 30, 2022

cc: @blink1073, RE: pre-commit.ci ⬆️

@blink1073
Copy link
Member

Yeah, basically we'd have to completely rethink a bunch of stuff to make this repo compatible with pre-commit.ci, so I removed it. cf jupyterlab/frontends-team-compass#142 (comment)

Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Nice cleanup, thanks @davidbrochart!

@github-actions
Copy link
Contributor

Benchmark report

The execution time (in milliseconds) are grouped by test file, test type and browser.
For each case, the following values are computed: min <- [1st quartile - median - 3rd quartile] -> max.

The mean relative comparison is computed with 95% confidence.

Results table
Test file large_code_notebook large_md_notebook
open
chromium
actual 3890 <- [4104 - 4190 - 4325] -> 4563 2568 <- [2735 - 2805 - 2863] -> 3220
expected 3923 <- [4150 - 4269 - 4337] -> 4591 2584 <- [2767 - 2830 - 2898] -> 3074
Mean relative change -0.8% ± 1.0% -0.9% ± 1.1%
switch-from
chromium
actual 625 <- [691 - 722 - 763] -> 1010 495 <- [550 - 952 - 1022] -> 1177
expected 669 <- [715 - 738 - 804] -> 1031 484 <- [534 - 961 - 1023] -> 1162
Mean relative change -4.1% ± 3.4% 3.6% ± 8.5%
switch-to
chromium
actual 320 <- [366 - 395 - 414] -> 442 233 <- [283 - 297 - 310] -> 395
expected 312 <- [379 - 407 - 422] -> 496 245 <- [287 - 297 - 310] -> 362
Mean relative change -2.2% ± 2.1% -0.5% ± 2.0%
close
chromium
actual 539 <- [958 - 995 - 1026] -> 1173 443 <- [480 - 501 - 516] -> 561
expected 606 <- [988 - 1012 - 1034] -> 1147 443 <- [487 - 512 - 526] -> 570
Mean relative change -1.7% ± 2.8% -1.4% ± 1.5%

Changes are computed with expected as reference.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-change A change that should be accompanied by a major version increase documentation enhancement pkg:docprovider pkg:docregistry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants