Refactor to use multistream_select

[dheatovwil]

I've forked the multistream_select repo, updated it with latest code in libp2p, and re-referenced the package.
I'm quite new so I'm not entirely sure is everything done, comments are appreciated.
multistream repo at dheatovwil/py-multistream-select

[robzajac]

Hi @dheatovwil, thanks for your contribution. Since the py-multistream-select repo simply uses the multistream-select code from py-libp2p, I'm not sure it needs to be included as a separate package and dependency. This also helps us stay consistent with the style of the rest of the repo (modules contained within the py-libp2p repo itself instead of separate repos). Let us know your thoughts.

[dheatovwil]

@robzajac
From py-ipfs/issues/49

Factor out https://github.com/zixuanzh/py-libp2p/tree/master/protocol_muxer into a separate py-multistream-select library and update py-libp2p to use it (Easy!)

* Current `py-multistream-select` library: https://github.com/ngetahun/py-multistream-select

I am not sure about @Alexander255's intention, from my observation it seems adhering to go-libp2p structure
Edit: according to the roadmap, multistream-select is declared as one of the supporting modules, specified by multiformats/multistream-select

[robzajac]

Thanks - we'll wait for followup from @Alexander255.

[ntninja]

@robzajac: The intention behind this is, afaik, to a self-sufficient and reusable client/server library for the multistream-select protocol. (Which indeed is a separate protocol, although intentionally minimalist.)
Imagine for instance someone wants to build a minimal custom IPFS client with as few lines of code as possible: They'd still likely need most of what the multiformats repositories provide since that is part of the core data model (CIDs most importantly); they'd also need a bitswap client, but could do without libp2p if they limit themselves to TCP and relay nodes (likely).
This case, I think, demonstrates that multistream-select (unlike most other libp2p support modules) indeed has a self-sufficient use-case. As far as I know the idea was also to be able to use MSS in protocols totally unrelated to IPFS et al since it provides a simple and robust way of negotiating protocol versions on connection start. Most libp2p modules on the other hand provide plumbing for connecting using this transport or that without the library user (passing its multiaddr) having to care – as such they are not self-sufficient and there'd be little point in separating them out into different projects.

Thanks for your work on this @dheatovwil, btw!

[dheatovwil]

No worries, so now I gotta package it to PyPI, is this how it works?
Should I merge my multistream-select to the original ngetahun/py-multistream-select?
Edit: Uploaded package to PyPI

[codecov-io]

Codecov Report

Merging #101 into master will decrease coverage by 1.87%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #101      +/-   ##
==========================================
- Coverage   57.19%   55.31%   -1.88%     
==========================================
  Files          53       47       -6     
  Lines        1668     1580      -88     
==========================================
- Hits          954      874      -80     
+ Misses        714      706       -8

Impacted Files	Coverage Δ
network/swarm.py	91.04% <100%> (ø)	⬆️
tests/protocol_muxer/test_protocol_muxer.py	100% <100%> (ø)	⬆️
tests/examples/test_chat.py	88.57% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9cfe26f...a6768e0. Read the comment docs.

[ntninja]

@dheatovwil: Yes, that's how it works 🙂

You likely also want to integrate https://github.com/zixuanzh/py-libp2p/blob/master/tests/protocol_muxer/test_protocol_muxer.py into your codebase.

I will look into getting your repo moved to the multiformats org. Please also add me and @robzajac as project owners on PyPI to increase the bus factor. (My handle is ntninja there & everywhere else.)

@robzajac: I hope you don't me/us moving ahead with this so quickly – I'm just excited to see some progress here. 😁

[ntninja]

BTW: I also recommend using flit rather than setuptools for packaging pure-python modules; the experience of packaging is much nicer with it.

Either way you'll want to set python_requires = ">=3.5" and drop the 2.7 trove classifiers. If you decide to continue using setuptools, please also add long_description_content_type="text/markdown" to setup.py to ensure that the description will be properly parsed by PyPI (not required with flit).

[robzajac]

CC @raulk

Thanks @Alexander255 for the details and @dheatovwil again for leading this effort. We see the value in a distinct multistream_select repo and we're excited as well that things are moving forward quickly. There are a few things we would want to see in order to feel comfortable using multistream_select as a dependency in py-libp2p:

1. Having the repo moved to the multiformats org so it becomes canonical.
2. Ownership access to this repo by the py-libp2p team, especially @stuckinaboot who wrote our implementation and whose input would be invaluable. We ask this so that py-libp2p development can proceed quickly in the event of a necessary multistream_select change and so the work of @stuckinaboot is recognized.
3. A passing Travis pipeline that incorporates this test like you mentioned above.

We hope this is reasonable, and we're excited to be moving forward!

P.S. @dheatovwil I am robzajac on PyPI.

[dheatovwil]

Thanks for the tips, updated the package and repo as you guys mentioned, working on the testcase now.

[ntninja]

@dheatovwil: Were you able to move forward on this since your last comment?

[dheatovwil]

@Alexander255 Sorry about the late reply, the progress will be a little slow due to other events going on at the same time, I think I can finalize everything in a week or two.

[dheatovwil]

@Alexander255 @robzajac I've requested to transfer the ownership to @stuckinaboot. I've also gone through the testcases and travis pipeline, everything seems fine. The testcase in this repo is updated to use the multistream-select package on pypi, and the coverage of that repo is at 100%. I've also updated with all previous changes and fixed the conflicts, is there anything else I am missing?

[ntninja]

@dheatovwil: LGTM. I looked over the source code and it appears to be well designed and the test are decent. However, as you are probably aware, messages are not actually length-encoded and there are no tests checking whether the correct wire format is actually produced. I also filed an issue on the multistream spec since I noticed that your source, just like everybody else's, assumes that messages are UTF-8 encoded, but that isn't actually written anywhere.

[stuckinaboot]

@dheatovwil Thanks for working on this. I looked through your code and the update you made to implement the 'ls' command and have one piece of feedback. As of now, when 'ls' is seen in negotiate of the multiselect.py file, you send back each supported protocol as it's own message. Was there a particular reason for that (maybe I missed something in the go code)? I believe that is less efficient because each message adds overhead and the other party would have to know how many messages to expect. Can you please update this so that a line-delimited list of protocols is sent in a single message when 'ls' is received (e.g. "protocol1\nprotocol2\nprotocol3")?

Also, in order to merge your PR, we'll need your py-multistream-select repo moved to the multiformats org so that it becomes canonical.

[dheatovwil]

@stuckinaboot Yes you are right, I've checked against go and js code, it's an honest mistake. I've fixed it.

Regarding that, I tried to transfer the ownership to multiformats but it says I don't have the permission to do so. Should I talk to the guys at multiformats regarding this issue?

[ntninja]

Bump. Has there been any update on the procedural issues around this?

[ralexstokes]

It seems this PR moves to extract our multistream-select implementation into another package -- i'm generally in favor of doing this (motivation higher up in the conversation) although it seems this PR has become a bit stale -- we have moved ahead w/ getting the multistream-select implementation in this repo into an interoperable state w/ the Go implementation.

i'd be happy to help facilitate the extraction but for now i'll close this PR. feel free to re-open if its best or make a new issue to track the multistream-select extraction :)