python-zulip-api icon indicating copy to clipboard operation
python-zulip-api copied to clipboard

interrealm: Add option to run the bridge unidirectionally.

Open rht opened this issue 5 years ago • 9 comments

This is for the use case of playground.zulipdev.org.

rht avatar Jul 25 '18 06:07 rht

Heads up @rht, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

zulipbot avatar Aug 13 '18 18:08 zulipbot

@neiljp can you take a look at this?

timabbott avatar Aug 13 '18 18:08 timabbott

@neiljp how does a concurrent.futures version look like? If it is cleaner, maybe it should be used in the other bridges.

rht avatar Jan 19 '19 19:01 rht

@rht I haven't explored it in detail for this application, but it could perhaps be something like the following (untested):

    from concurrent.futures import ProcessPoolExecutor, wait
    pipe_event1 = create_pipe_event(client2, bot1, bot2, args.stream)
    with ProcessPoolExecutor(max_workers=2) as executor:
        f = [executor.submit(client1.call_on_each_event, pipe_event1, ["message"])]
        if not args.oneway:
            pipe_event2 = create_pipe_event(client1, bot2, bot1, args.stream)
            f += [executor.submit(client2.call_on_each_event, pipe_event2, ["message"])]
        print("Listening...")
        wait(f)

I used this module in zulip-terminal recently.

neiljp avatar Jan 19 '19 19:01 neiljp

That looks like a bit longer than I thought (concurrent.futures as a common interface to multiprocessing and threading). Would it be possible to do

f1 = executor.submit(client1.call_on_each_event, pipe_event1, ["message"])
f1.future()

? Even if this is possible, there is still create_pipe_event construct, which is another extra structure.

rht avatar Jan 19 '19 23:01 rht

Longer? The original is 13 lines, and the new one is 8, plus the import line - plus cleaner, in my opinion, though no guarantee it's directly equivalent as I'm not completely familiar with the existing code.

Re your question, f1 is already a future; in my draft code above, the futures are just being used to test the completion state of the code, which by my understanding is equivalent to a join. I found the library docs quite readable - assuming I understand them, of course ;)

Refactoring for the other structures is a separate step, which I hadn't considered.

neiljp avatar Jan 20 '19 00:01 neiljp

"Longer" in a horizontal way rather than vertical way, in terms of the number of nested expressions. (Also, I somehow forgot the fact that create_pipe_event was a construct in the bridge itself.) Also, typo: f1.result() instead of f1.future().

Aside: maybe they (anything that runs n functions in parallel) could be wrapped in a run_parallel so that no one has to see the gory detail of the parallelization?

run_parallel((func1, args1), (func2, args2), ...)

There is a run_parallel function in zulip/zulip, but it uses os process forks.

rht avatar Jan 20 '19 00:01 rht

The extra level of nesting is due to that context manager, which tidies things up automatically - does the same happen currently?

You could get a result by f1.result(), but currently the result of call_on_each_event would be None, of course; I think it would probably block at that point, too.

neiljp avatar Jan 20 '19 01:01 neiljp

Heads up @rht, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

zulipbot avatar Apr 10 '20 00:04 zulipbot