Skip to content

Avoid signaling outside main thread when starting jupyter server#7033

Open
bnaul wants to merge 5 commits intodask:mainfrom
replicahq:signal_main_thread
Open

Avoid signaling outside main thread when starting jupyter server#7033
bnaul wants to merge 5 commits intodask:mainfrom
replicahq:signal_main_thread

Conversation

@bnaul
Copy link
Copy Markdown
Contributor

@bnaul bnaul commented Sep 14, 2022

Closes #6886

@mrocklin dunno if I was taking @Carreau too literally here but this does resolve the issue (i.e. the example from the issue now succeeds). Not sure if it's bad form to be actually overriding these class methods, maybe we want to subclass or something?

@bnaul bnaul changed the title Avoid signaling outside main thread Avoid signaling outside main thread when starting jupyter server Sep 14, 2022
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Sep 14, 2022

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

       15 files  ±  0         15 suites  ±0   6h 26m 26s ⏱️ + 13m 13s
  3 107 tests +  6    3 021 ✔️ +  6    85 💤 ±0  1 ±0 
22 997 runs  +43  22 090 ✔️ +43  906 💤 ±0  1 ±0 

For more details on these failures, see this check.

Results for commit 8439f35. ± Comparison against base commit 1fd07f0.

♻️ This comment has been updated with latest results.

@Carreau
Copy link
Copy Markdown
Contributor

Carreau commented Sep 14, 2022

You took me literally enough.
Though I think you need to patch the class (or subclass) before creating the instance.

@bnaul
Copy link
Copy Markdown
Contributor Author

bnaul commented Sep 14, 2022

Huh I would have thought so too but evidently not...

[ins] In [23]: class A():
          ...:     def f(self):
          ...:         print("A")
          ...: a = A()
          ...: a.f()
          ...:
          ...: A.f = lambda self: print("B")
          ...: a.f()
A
B

but regardless yeah I agree that seems like the right thing to do

@bnaul
Copy link
Copy Markdown
Contributor Author

bnaul commented Sep 19, 2022

@mrocklin any thoughts on this? obviously kind of a niche use case within a niche use case but seems pretty safe to me...?

Copy link
Copy Markdown
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @bnaul! Would you mind adding some kind of test for this?

It sounds like @Carreau is happy with the changes here. @ian-r-rose do you have any thoughts?

@ian-r-rose
Copy link
Copy Markdown
Collaborator

Seems reasonable to me -- I'd like an explanatory comment, but otherwise LGTM

@bnaul
Copy link
Copy Markdown
Contributor Author

bnaul commented Sep 19, 2022

Thanks @jrbourbeau and @ian-r-rose, I added a simple test and comment

@bnaul bnaul requested a review from fjetter as a code owner January 23, 2024 10:57
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.

Jupyter server won't start from SpecCluster

4 participants