#414 [python] provide a way to wait until builds finish
Merged 5 years ago by frostyx. Opened 5 years ago by frostyx.
copr/ frostyx/copr apiv3-watch-build  into  master

@@ -0,0 +1,62 @@ 

+ import pytest

+ import mock

+ from munch import Munch

+ from copr.v3.helpers import wait, succeeded

+ from copr.v3 import BuildProxy, CoprException

+ 

+ 

+ class TestHelpers(object):

+     def test_succeeded(self):

+         b1 = Munch(state="succeeded")

+         b2 = Munch(state="succeeded")

+         b3 = Munch(state="running")

+         b4 = Munch(state="failed")

+ 

+         assert succeeded(b1)

+         assert succeeded([b1, b2])

+         assert not succeeded(b3)

+         assert not succeeded([b1, b2, b4])

+ 

+ 

+ class TestWait(object):

+     @mock.patch("copr.v3.proxies.build.BuildProxy.get")

+     def test_wait(self, mock_get):

+         build = MunchMock(id=1, state="importing")

+ 

+         mock_get.return_value = MunchMock(id=1, state="succeeded")

+         assert wait(build)

+ 

+         mock_get.return_value = MunchMock(id=1, state="unknown")

+         with pytest.raises(CoprException) as ex:

+             wait(build)

+         assert "Unknown status" in str(ex)

+ 

+     @mock.patch("copr.v3.proxies.build.BuildProxy.get")

+     def test_wait_list(self, mock_get):

+         builds = [MunchMock(id=1, state="succeeded"), MunchMock(id=2, state="failed")]

+         mock_get.side_effect = lambda id: builds[id-1]

+         assert wait(builds)

+ 

+     @mock.patch("time.time")

+     @mock.patch("copr.v3.proxies.build.BuildProxy.get")

+     def test_wait_timeout(self, mock_get, mock_time):

+         build = MunchMock(id=1, state="importing")

+ 

+         mock_get.return_value = MunchMock(id=1, state="running")

+         mock_time.return_value = 0

+         with pytest.raises(CoprException) as ex:

+             wait(build, interval=0, timeout=-10)

+         assert "Timeouted" in str(ex)

+ 

+     @mock.patch("copr.v3.proxies.build.BuildProxy.get")

+     def test_wait_callback(self, mock_get):

+         build = MunchMock(id=1, state="importing")

+ 

+         callback = mock.Mock()

+         mock_get.return_value = MunchMock(id=1, state="failed")

+         wait(build, interval=0, callback=callback)

+         assert callback.called

+ 

+ 

+ class MunchMock(Munch):

+     __proxy__ = BuildProxy({})

file modified
+62 -1
@@ -1,8 +1,9 @@ 

  import os

  import six

+ import time

  import configparser

  from munch import Munch

- from .exceptions import CoprNoConfigException, CoprConfigException

+ from .exceptions import CoprNoConfigException, CoprConfigException, CoprException

  

  

  class List(list):
@@ -59,3 +60,63 @@ 

          result.__proxy__ = args[0]

          return result

      return wrapper

+ 

+ 

+ def wait(waitable, interval=30, callback=None, timeout=0):

+     """

+     Wait for a waitable thing to finish. At this point, it is possible to wait only

+     for builds, but this function should be enhanced to wait for

+     e.g. modules or images, etc in the future

+ 

+     :param Munch/list waitable: A Munch result or list of munches

+     :param int interval: How many seconds wait before requesting updated Munches from frontend

+     :param callable callback: Callable taking one argument (list of build Munches).

+                               It will be triggered before every sleep interval.

+     :param int timeout: Limit how many seconds should be waited before this function unsuccessfully ends

+     :return: list of build Munches

+ 

+     Example usage:

+ 

+         build1 = client.build_proxy.create_from_file(...)

+         build2 = client.build_proxy.create_from_scm(...)

+         wait([build1, build2])

+ 

+     """

+     builds = waitable if type(waitable) == list else [waitable]

+     watched = set([build.id for build in builds])

+     munches = {build.id: build for build in builds}

+     failed = []

+     terminate = time.time() + timeout

+ 

+     while True:

+         for build_id in watched.copy():

+             build = munches[build_id] = munches[build_id].__proxy__.get(build_id)

+             if build.state in ["failed"]:

+                 failed.append(build_id)

+             if build.state in ["succeeded", "skipped", "failed", "canceled"]:

+                 watched.remove(build_id)

+             if build.state == "unknown":

+                 raise CoprException("Unknown status.")

+ 

+         if callback:

+             callback(list(munches.values()))

+         if not watched:

+             break

+         if timeout and time.time() >= terminate:

+             raise CoprException("Timeouted")

+         time.sleep(interval)

+     return list(munches.values())

+ 

+ 

+ def succeeded(builds):

+     """

+     Determine, whether the list of builds finished successfully.

+ 

+     :param Munch/list builds: A list of builds or a single build Munch

+     :return bool:

+     """

+     builds = builds if type(builds) == list else [builds]

+     for build in builds:

+         if build.state != "succeeded":

+             return False

+     return True

This is supposed to solve RhBug: 1258970
https://bugzilla.redhat.com/show_bug.cgi?id=1258970

It is just a preview, I want to add also unit tests before this gets merged.
There are a couple of things, that I want to discuss.

1) Name of the function - wait is very general, which can be good and bad. It could be modified in the future, to wait for various things, such as modules, containers, whatever. Do you prefer this, or rather more explicit watch_builds, wait_for_builds, etc?
2) build_proxy parameter. We agreed, that this function would fit in helpers file. However, in such case, it will require a build_proxy, config or client parameter, because otherwise there is no way to request the current build state from frontend. The other option is not having it as a helper, but rather in BuildProxy (i.e. BuildProxy.wait(...)).

Also, example usage is here:

from copr.v3 import Client
from copr.v3.helpers import wait, succeeded

def watch_progress(watched):
    print("Progress: {}".format(", ".join(["#{} {}".format(b.id, b.state) for b in watched])))

client = Client.create_from_config_file()
hello = "https://frostyx.fedorapeople.org/hello-2.8-1.fc23.src.rpm"
builds = client.build_proxy.create_from_urls("frostyx", "hello", [hello, hello])
builds = wait(builds, client.build_proxy, callback=watch_progress)
print("Builds {} successful".format("were" if succeeded(builds) else "weren't"))

It could be modified in the future, to wait for various things, such as modules, containers, whatever.

Shouldn't there be some inheritance, then? I mean, some generic class with abstract method providing state of the object, and you could wait(child_of_the_class)? This would solve the second point..

However, in such case, it will require a build_proxy, config or client parameter

... where you could include the abstract class only.

Otherwise, since I'm not an user of the API, I pretty like it and I don't see serious issues.

I'd prefer to have some form of timeout, too. Either specified by number of attempts, or the maximum timeout in seconds.

I'd prefer to have some form of timeout, too. Either specified by number of attempts, or the maximum timeout in seconds.

Yea, I totally forgot. Some timeout needs to be implemented.

I have the timeout option already implemented. I want to write some tests and push it.
What bothers me and therefore blocks this PR is the build_proxy parameter.
I would like to have PR#436 or find another solution.

4 new commits added

  • [python] accept both one munch result or list of them
  • [python] add tests for wait and succeeded functions
  • [python] don't fail when no callback
  • [frontend] add timeout
5 years ago

I have added the timeout option and some unit tests.
The last missing piece is the build_proxy parameter, which I would really like to get rid of.

  • the method should be called wait_for_builds() or so, instead of wait()
  • I'd be +1 for moving that method to client directly, or to some client.helpers.wait_builds() namespace (because yes, proxies are not really good place to put this logic)

I'd be +1 for moving that method to client directly, or to some client.helpers.wait_builds() namespace (because yes, proxies are not really good place to put this logic)

It should be in BuildProxy if anywhere. Client is just a wrapper over all proxies and you can access any build-related method from there.

It should be in BuildProxy if anywhere.

Well, I got @frostyx's point that proxies are supposed to follow KISS principle. And so, I believe that having the waiting logic elsewhere makes much more sense.

The problem really seems to be very similar to how we understand "models" vs. "logic" classes internally; the first is concentrated on small and narrow units (we never do long processing there) and the other works on top of that, doing more expensive tasks.

Client is just a wrapper over all proxies and you can access any build-related method from there.

It is the current status, but I wouldn't claim that it is what users expect, really; I mean, if they instantiate Client, they probably expect that the class does something. Note also that I proposed client.helpers namespace (and you can think about nicer names, say client.utils, ...).

I'm just saying what is my preference, what I think would be nicer. It would be pitty to hang here on such trivial thing.

I would say, let's figure the PR#436 first. If it can be finished and merged, finishing this PR will be trivial. If we decide to close it, then we need to figure out what to do here. @msuchy, can you please take a look on PR#436 and cut it?

@frostyx can this be merged? Or you want to change something?

Do not merge, please. PR#436 were finally merged, so I need to modify this to use the feature.

rebased onto 391a712

5 years ago

1 new commit added

  • [python] don't pass proxy object to the wait method
5 years ago

I've rebased this PR and resolved conflicts.
Also, I've updated the wait function, so a user doesn't have to pass a proxy object to it.

the method should be called wait_for_builds() or so, instead of wait()

My idea was to have a smart wait function, that will understand on what you want to wait. IDK whether it is a bad idea, ... I like it though. Let me explain it to you and figure it out. We would have

def wait(result, interval=30, callback=None, timeout=0):
def is_build_finished(build):

and if we would call wait function with a build munch (or list of them), it would periodically call is_build_finished(...) on them. Later if we would want to wait for modules it would similarly call is_module_finished(...), etc. If we would want to do something in the future and for example have some objects that could be waited on, they would implement is_finished(self, ...) method and our wait function would call that.

I see the benefit, that user will have to care about just one function for waiting on something. Also, if written well, it should be possible to pass a list of different munches/objects to the wait function and it should handle it.

But if it sounds like a lot of unnecessary magic, we can just rename the current wait function to proposed wait_for_builds(...) and that's it.

1 new commit added

  • [python] improve the documentation for wait and succeeded functions
5 years ago

As I understood from the last meeting, there are mixed feelings about the currently proposed wait method, but no one particularly disagrees with it. There was a suggestion to improve its documentation, but it should be ready to go.

I improved the documentation and also added an example usage to it and therefore I am merging this PR. Please feel free to open an issue, if you find any problem with it.

Pull-Request has been merged by frostyx

5 years ago