Re: [viff-devel] Broken unit tests
Martin Geisler wrote: I'm very confused about what exactly for x in xs[i:] + xs[:i]: ... i = (i + 1) % len(xs) is supposed to do? After x has run through all xs (rotated i steps), then i will have been incremented by len(xs). But you do it mod len(xs) and so i comes out of the loop unchanged. This solution is indeed not very elegant. The problem that has to be solved here is the following: In a normal run every player (i.e. every runtime) has its own reactor and this reactor runs process_deferred_queue() of one runtime. In the unit tests however, there is one reactor for n runtimes. Therefore the reactor has to call process_deferred_queue() of every runtime. Now, if we just would iterate as usual over the runtimes this would lead to very unbalanced and therefore slower execution, because the loop call is called recursively. So this construct is to ensure that every time loop_call() is called, another runtime has first priority. Is all this even necessary when the code calls self.activate_reactor once in a while? Yes because activate_reactor() lets the reactor call the loop call. I sort of had the impression that it was not necessary to activate the reactor explicitly -- do things no longer work correctly without it? The loop call is something different than activating the reactor. If reactor is a ViffReactor a proper loop call is mandatory, but not the reactor activation. In the case of a SelectReactor activate_reactor() shouldn't do anything, this is a mistake, which I will correct. I'll also make a patch to make the unit tests also run with a SelectReactor. Also, the sending of messages is already delayed by a random amount, so perhaps the unbalanced execution will be handled that way? I don't think so. ___ viff-devel mailing list (http://viff.dk/) viff-devel@viff.dk http://lists.viff.dk/listinfo.cgi/viff-devel-viff.dk
Re: [viff-devel] Broken unit tests
Marcel Keller writes: > Hi Martin, > > Martin Geisler wrote: >> All the tests in the buildbot are currently broken: >> >> http://buildbot.viff.dk/waterfall >> >> It is because of this change of yours: >> >> @@ -164,6 +166,18 @@ >> # the Runtime, since we want everybody to wait until all >> # runtimes are ready. >> self.runtimes.append(result) >> +self.real_runtimes.append(runtime) >> +self.i = 0 >> + >> +# This loop call should ensure the queues of the parties are >> +# processed in a more or less fair manner. This is necessary >> +# because we have only one reactor for all parties here. >> +def loop_call(): >> +for runtime in self.real_runtimes[self.i:] + >> self.real_runtimes[:self.i]: >> +runtime.process_deferred_queue() >> +self.i = (self.i + 1) % len(self.real_runtimes) >> + >> +reactor.setLoopCall(loop_call) > > I think the problem is that after this change the unit tests need the > reactor to be a ViffReactor, but it seems that the reactor is a > SelectReactor. And this seems to be the case since your changeset > "Make the VIFF reactor available to trial". Okay, I see... this is a bit strange. I remember talking with Thomas about it, and we found some sort of problem with installing the VIFF reactor viff/test/__init__.py. I think the code was not loaded under some circumstances, but I cannot figure out which at the moment :-( >> I'm very confused about what exactly >> >> for x in xs[i:] + xs[:i]: >> ... >> i = (i + 1) % len(xs) >> >> is supposed to do? After x has run through all xs (rotated i steps), >> then i will have been incremented by len(xs). But you do it mod >> len(xs) and so i comes out of the loop unchanged. > > This solution is indeed not very elegant. The problem that has to be > solved here is the following: In a normal run every player (i.e. every > runtime) has its own reactor and this reactor runs > process_deferred_queue() of one runtime. In the unit tests however, > there is one reactor for n runtimes. Therefore the reactor has to call > process_deferred_queue() of every runtime. Now, if we just would > iterate as usual over the runtimes this would lead to very unbalanced > and therefore slower execution, because the loop call is called > recursively. So this construct is to ensure that every time > loop_call() is called, another runtime has first priority. Is all this even necessary when the code calls self.activate_reactor once in a while? I sort of had the impression that it was not necessary to activate the reactor explicitly -- do things no longer work correctly without it? Also, the sending of messages is already delayed by a random amount, so perhaps the unbalanced execution will be handled that way? -- Martin Geisler ___ viff-devel mailing list (http://viff.dk/) viff-devel@viff.dk http://lists.viff.dk/listinfo.cgi/viff-devel-viff.dk
Re: [viff-devel] Broken unit tests
Hi Martin, Martin Geisler wrote: All the tests in the buildbot are currently broken: http://buildbot.viff.dk/waterfall It is because of this change of yours: @@ -164,6 +166,18 @@ # the Runtime, since we want everybody to wait until all # runtimes are ready. self.runtimes.append(result) +self.real_runtimes.append(runtime) +self.i = 0 + +# This loop call should ensure the queues of the parties are +# processed in a more or less fair manner. This is necessary +# because we have only one reactor for all parties here. +def loop_call(): +for runtime in self.real_runtimes[self.i:] + self.real_runtimes[:self.i]: +runtime.process_deferred_queue() +self.i = (self.i + 1) % len(self.real_runtimes) + +reactor.setLoopCall(loop_call) I think the problem is that after this change the unit tests need the reactor to be a ViffReactor, but it seems that the reactor is a SelectReactor. And this seems to be the case since your changeset "Make the VIFF reactor available to trial". I'm very confused about what exactly for x in xs[i:] + xs[:i]: ... i = (i + 1) % len(xs) is supposed to do? After x has run through all xs (rotated i steps), then i will have been incremented by len(xs). But you do it mod len(xs) and so i comes out of the loop unchanged. This solution is indeed not very elegant. The problem that has to be solved here is the following: In a normal run every player (i.e. every runtime) has its own reactor and this reactor runs process_deferred_queue() of one runtime. In the unit tests however, there is one reactor for n runtimes. Therefore the reactor has to call process_deferred_queue() of every runtime. Now, if we just would iterate as usual over the runtimes this would lead to very unbalanced and therefore slower execution, because the loop call is called recursively. So this construct is to ensure that every time loop_call() is called, another runtime has first priority. The xs[i:] + xs[:i] construct is at best uncommon and at worst wasted work. Instead of rotating the list, it should be enough to start at an offset (i) and work your way through the list with proper wrapping at the end. You're right. Also, the fact that you have both self.runtimes and self.real_runtimes smells funny :-) It's as if you're circumventing the natural scopes of the Deferreds by storing a reference to the runtimes "on the side". The list of Deferreds in self.runtimes ought to be enough -- can you not add callbacks to that list if you need something to happen when the runtimes are ready? Yes, it just was too lazy. Best reagards, Marcel ___ viff-devel mailing list (http://viff.dk/) viff-devel@viff.dk http://lists.viff.dk/listinfo.cgi/viff-devel-viff.dk
[viff-devel] Broken unit tests
Hi Marcel, All the tests in the buildbot are currently broken: http://buildbot.viff.dk/waterfall It is because of this change of yours: @@ -164,6 +166,18 @@ # the Runtime, since we want everybody to wait until all # runtimes are ready. self.runtimes.append(result) +self.real_runtimes.append(runtime) +self.i = 0 + +# This loop call should ensure the queues of the parties are +# processed in a more or less fair manner. This is necessary +# because we have only one reactor for all parties here. +def loop_call(): +for runtime in self.real_runtimes[self.i:] + self.real_runtimes[:self.i]: +runtime.process_deferred_queue() +self.i = (self.i + 1) % len(self.real_runtimes) + +reactor.setLoopCall(loop_call) I'm very confused about what exactly for x in xs[i:] + xs[:i]: ... i = (i + 1) % len(xs) is supposed to do? After x has run through all xs (rotated i steps), then i will have been incremented by len(xs). But you do it mod len(xs) and so i comes out of the loop unchanged. The xs[i:] + xs[:i] construct is at best uncommon and at worst wasted work. Instead of rotating the list, it should be enough to start at an offset (i) and work your way through the list with proper wrapping at the end. Also, the fact that you have both self.runtimes and self.real_runtimes smells funny :-) It's as if you're circumventing the natural scopes of the Deferreds by storing a reference to the runtimes "on the side". The list of Deferreds in self.runtimes ought to be enough -- can you not add callbacks to that list if you need something to happen when the runtimes are ready? -- Martin Geisler VIFF (Virtual Ideal Functionality Framework) brings easy and efficient SMPC (Secure Multiparty Computation) to Python. See: http://viff.dk/. ___ viff-devel mailing list (http://viff.dk/) viff-devel@viff.dk http://lists.viff.dk/listinfo.cgi/viff-devel-viff.dk
