distributed icon indicating copy to clipboard operation
distributed copied to clipboard

Avoid signaling outside main thread when starting jupyter server

Open bnaul opened this issue 3 years ago • 6 comments

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 avatar Sep 14 '22 00:09 bnaul

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 :stopwatch: + 13m 13s   3 107 tests +  6    3 021 :heavy_check_mark: +  6    85 :zzz: ±0  1 :x: ±0  22 997 runs  +43  22 090 :heavy_check_mark: +43  906 :zzz: ±0  1 :x: ±0 

For more details on these failures, see this check.

Results for commit 8439f353. ± Comparison against base commit 1fd07f03.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Sep 14 '22 01:09 github-actions[bot]

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

Carreau avatar Sep 14 '22 08:09 Carreau

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 avatar Sep 14 '22 13:09 bnaul

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

bnaul avatar Sep 19 '22 13:09 bnaul

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

ian-r-rose avatar Sep 19 '22 15:09 ian-r-rose

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

bnaul avatar Sep 19 '22 21:09 bnaul