Fix IPC tests and documentation

[Flood1993]

No description provided.

[codecov]

Codecov Report

Merging #147 into master will decrease coverage by 0.09%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #147     +/-   ##
=========================================
- Coverage   98.87%   98.77%   -0.1%     
=========================================
  Files          25       25             
  Lines        3196     3195      -1     
  Branches      241      241             
=========================================
- Hits         3160     3156      -4     
- Misses         24       27      +3     
  Partials       12       12

Impacted Files	Coverage Δ
osbrain/tests/test_agent_transport.py	96.38% <100%> (+0.28%)	⬆️
osbrain/agent.py	96.17% <0%> (-0.45%)	⬇️
osbrain/tests/common.py	100% <0%> (ø)	⬆️
osbrain/tests/test_common.py	100% <0%> (ø)	⬆️

---

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 59ac851...d5f3be3. Read the comment docs.

[Peque]

Maybe you could rebase onto #146 so that we can see the AppVeyor results as we tune our tests for Windows compatibility.

[Peque]

Let us keep an eye on irmen/Pyro4#179. That may help us fixing serialization problems in Windows.

[Peque]

Can you interactively rebase your 2 commits onto #146 again?

[Peque]

Once you do that, could you check if #172 helps with those serialization problems you were having?

If it does not, do you have a reproducible test case that we can share with cloudpickle team for them to fix it?

[Peque]

#196 is needed before trying anything, as many things were added/fixed in cloudpickle 0.4.0.

[ocaballeror]

Turns out the cause of all of these serialization problems is the way Windows creates new processes. Instead of forking (like *nix systems do), Windows spawns a new process and passes a bunch of information to it, which requires all the local objects to be picklable. Since we use a lot of nested classes for these tests, and nested classes can't be pickled, a pickling error will be raised for every single test that uses one.

Since the errors are raised from the standard multiprocessing module, there's not much we can control about this. The way I see it we have two options:

• Replace the standard pickle module with cloudpickle at the start. I'm not sure it can be done, but if it can, it still sounds tremendously hacky to me.
• Refactor all the tests that use nested classes to either move these definitions to the top level of the module, or stop using nested classes altogether and use standard agents/nameservers/whatever with added methods. From my (very shallow) testing I can confirm the first method works.

@Peque thoughts?

[ocaballeror]

@Peque By the way, this seems like it's exceeding the purpose of this PR. Should I create a new one to discuss all of this?

[Peque]

@ocaballeror So basically it is failing due to the arguments passed to run_agent() not being serializable with pickle? If that is the case:

• Could we serialize them (i.e.: with dill) and store them serialized in AgentProcess and then de-serialize them in the run() method? (once the process has been forked)
• Would it make sense to use something like multiprocess. I would not be very excited about this, as adding a dependency and not using the standard library's multiprocessing might not be a good idea.

I think we can discuss everything in this PR. The idea is, somehow, to end up with AppVeyor in green here. 😄

[Peque]

PS: we probably only need to serialize base and attributes for now. Maybe even just base for now...

[ocaballeror]

@Peque that sounds much simpler 😄

I'm currently testing that solution and it seems to work just fine 👍 . I haven't finished with all the tests, though, but most of them are passing right now, which is more than I could say an hour ago.

[ocaballeror]

UPDATE: After multiple hours of testing, it turns out Windows behaves strangely (I know, shocker!) when dealing with socket addresses, and lets you bind multiple sockets to the same address and have them all listen at the same time. According to Microsoft's documentation, instead of raising an error, this setup will have "undefined behavior".

This seems to be the problem behind most of the currently failing tests, so I'll have to investigate how to work around this.

[ocaballeror]

Some documentation to back up my claims:

https://msdn.microsoft.com/en-us/library/windows/desktop/ms740621(v=vs.85).aspx
https://bugs.python.org/issue2550

It would be nice if there was even a mention to this in the python documentation

[Peque]

@ocaballeror Can you update the PR with the changes you have made so far? (at least those that seem to improve things). In example: serialization of the base class, separation of IPC tests to avoid executing them in AppVeyor...

[ocaballeror]

@Peque This is not my PR, so I guess I'll have to create a new one.

[Peque]

@ocaballeror Try first, just in case... 😊

In my case, for example, I have GitHub configured so that people reviewing my PRs can push commits to it.

[Peque]

@ocaballeror But yeah, if you cannot, just open a new one (and close this).

[ocaballeror]

Continues on #252