Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2451553006:
[Common] Move WaitFor from telemetry to common/py_utils (Closed)
|
Created:
4 years, 1 month ago by rnephew (Reviews Here) Modified:
4 years, 1 month ago CC:
catapult-reviews_chromium.org Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
Description[Common] Move WaitFor from telemetry to common/py_utils
BUG=catapult:#2955
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/6bdd081cfd114ef91e5c5518afd6bbd13dff9a6f
Patch Set 1 : [Common] Move WaitFor from telemetry to common/py_utils #
Total comments: 13
Patch Set 2 : [Common] Move WaitFor from telemetry to common/py_utils #
Total comments: 8
Patch Set 3 : Make telemetry calls use new version #Patch Set 4 : Charlies comments #
Total comments: 5
Patch Set 5 : undo telemetry changes #
Total comments: 1
Patch Set 6 : change times in unittetss #
Total comments: 2
Patch Set 7 : rebase #Patch Set 8 : nix logging #
Messages
Total messages: 30 (11 generated)
Patchset #1 (id:1) has been deleted
rnephew@chromium.org changed reviewers: + charliea@chromium.org, nednguyen@google.com
https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... File common/py_utils/py_utils/py_utils_unittest.py (right): https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... common/py_utils/py_utils/py_utils_unittest.py:37: def _ReturnCounterBasedValue(self): Anyone have a suggestion on a better implementation of a condition that will start out false and eventually return true?
https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... File common/py_utils/py_utils/__init__.py (right): https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... common/py_utils/py_utils/__init__.py:101: min_poll_interval = 0.1 Should these be constants? https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... common/py_utils/py_utils/__init__.py:101: min_poll_interval = 0.1 It's probably worth attaching units to these (MIN_POLL_INTERVAL_IN_SECONDS) or something like that see: https://cs.corp.google.com/search/?q=p:github+f:%5Ecatapult-project/catapult/... https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... common/py_utils/py_utils/__init__.py:105: def GetConditionString(): I'm confused: what's going on here? https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... common/py_utils/py_utils/__init__.py:115: while True: Maybe this could be more simply expressed as: condition_met = False while elapsed_time < timeout: if condition(): return # Sleep until next poll... raise TimeoutException(...) I'm generally against infinite loops (where possible), because: 1) I think putting the termination condition in the normal place makes it more obvious what that termination condition is 2) I think that it's easier to accidentally create actual infinite loops with them https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... File common/py_utils/py_utils/py_utils_unittest.py (right): https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... common/py_utils/py_utils/py_utils_unittest.py:37: def _ReturnCounterBasedValue(self): On 2016/10/25 21:53:56, rnephew (Reviews Here) wrote: > Anyone have a suggestion on a better implementation of a condition that will > start out false and eventually return true? I think this one is okay, but I might put the counter variable down in the test and make this function local to that test. Having the counter be a member of the class feels like we're leaking implementation details unnecessarily https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... common/py_utils/py_utils/py_utils_unittest.py:47: py_utils.WaitFor(self._ReturnFalse, 1) I wonder if isn't clearer to just have: py_utils.WaitFor(lambda: False, 1)
https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... File common/py_utils/py_utils/__init__.py (right): https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... common/py_utils/py_utils/__init__.py:101: min_poll_interval = 0.1 On 2016/10/26 15:19:32, charliea wrote: > Should these be constants? Done. https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... common/py_utils/py_utils/__init__.py:101: min_poll_interval = 0.1 On 2016/10/26 15:19:32, charliea wrote: > It's probably worth attaching units to these (MIN_POLL_INTERVAL_IN_SECONDS) or > something like that > > see: > https://cs.corp.google.com/search/?q=p:github+f:%5Ecatapult-project/catapult/... Done. https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... common/py_utils/py_utils/__init__.py:105: def GetConditionString(): On 2016/10/26 15:19:32, charliea wrote: > I'm confused: what's going on here? I just c/p the old code from telemetry and tested that it worked as expected.. I dont fully know what this code is for. https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... common/py_utils/py_utils/__init__.py:115: while True: On 2016/10/26 15:19:32, charliea wrote: > Maybe this could be more simply expressed as: > > condition_met = False > while elapsed_time < timeout: > if condition(): > return > > # Sleep until next poll... > > raise TimeoutException(...) > > I'm generally against infinite loops (where possible), because: > > 1) I think putting the termination condition in the normal place makes it more > obvious what that termination condition is > > 2) I think that it's easier to accidentally create actual infinite loops with > them Done. https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... File common/py_utils/py_utils/py_utils_unittest.py (right): https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... common/py_utils/py_utils/py_utils_unittest.py:37: def _ReturnCounterBasedValue(self): On 2016/10/26 15:19:32, charliea wrote: > On 2016/10/25 21:53:56, rnephew (Reviews Here) wrote: > > Anyone have a suggestion on a better implementation of a condition that will > > start out false and eventually return true? > > I think this one is okay, but I might put the counter variable down in the test > and make this function local to that test. Having the counter be a member of > the class feels like we're leaking implementation details unnecessarily Done, but to make the variable live in the test function, it needs to be passed as either a list or dict because of strange python scoping issues. https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... common/py_utils/py_utils/py_utils_unittest.py:47: py_utils.WaitFor(self._ReturnFalse, 1) On 2016/10/26 15:19:32, charliea wrote: > I wonder if isn't clearer to just have: > > py_utils.WaitFor(lambda: False, 1) I actually am adding some lambda based tests. Based on GetConditionString I think the code I'm porting over handles lambdas differently.
On 2016/10/26 16:26:05, rnephew (Reviews Here) wrote: > https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... > File common/py_utils/py_utils/__init__.py (right): > > https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... > common/py_utils/py_utils/__init__.py:101: min_poll_interval = 0.1 > On 2016/10/26 15:19:32, charliea wrote: > > Should these be constants? > > Done. > > https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... > common/py_utils/py_utils/__init__.py:101: min_poll_interval = 0.1 > On 2016/10/26 15:19:32, charliea wrote: > > It's probably worth attaching units to these (MIN_POLL_INTERVAL_IN_SECONDS) or > > something like that > > > > see: > > > https://cs.corp.google.com/search/?q=p:github+f:%5Ecatapult-project/catapult/... > > Done. > > https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... > common/py_utils/py_utils/__init__.py:105: def GetConditionString(): > On 2016/10/26 15:19:32, charliea wrote: > > I'm confused: what's going on here? > > I just c/p the old code from telemetry and tested that it worked as expected.. I > dont fully know what this code is for. > > https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... > common/py_utils/py_utils/__init__.py:115: while True: > On 2016/10/26 15:19:32, charliea wrote: > > Maybe this could be more simply expressed as: > > > > condition_met = False > > while elapsed_time < timeout: > > if condition(): > > return > > > > # Sleep until next poll... > > > > raise TimeoutException(...) > > > > I'm generally against infinite loops (where possible), because: > > > > 1) I think putting the termination condition in the normal place makes it more > > obvious what that termination condition is > > > > 2) I think that it's easier to accidentally create actual infinite loops with > > them > > Done. > > https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... > File common/py_utils/py_utils/py_utils_unittest.py (right): > > https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... > common/py_utils/py_utils/py_utils_unittest.py:37: def > _ReturnCounterBasedValue(self): > On 2016/10/26 15:19:32, charliea wrote: > > On 2016/10/25 21:53:56, rnephew (Reviews Here) wrote: > > > Anyone have a suggestion on a better implementation of a condition that will > > > start out false and eventually return true? > > > > I think this one is okay, but I might put the counter variable down in the > test > > and make this function local to that test. Having the counter be a member of > > the class feels like we're leaking implementation details unnecessarily > > Done, but to make the variable live in the test function, it needs to be passed > as either a list or dict because of strange python scoping issues. > > https://codereview.chromium.org/2451553006/diff/20001/common/py_utils/py_util... > common/py_utils/py_utils/py_utils_unittest.py:47: > py_utils.WaitFor(self._ReturnFalse, 1) > On 2016/10/26 15:19:32, charliea wrote: > > I wonder if isn't clearer to just have: > > > > py_utils.WaitFor(lambda: False, 1) > > I actually am adding some lambda based tests. Based on GetConditionString I > think the code I'm porting over handles lambdas differently. Can you also update the code in Telemetry to use this one?
https://codereview.chromium.org/2451553006/diff/40001/common/py_utils/py_util... File common/py_utils/py_utils/__init__.py (right): https://codereview.chromium.org/2451553006/diff/40001/common/py_utils/py_util... common/py_utils/py_utils/__init__.py:97: def WaitFor(condition, timeout): It seems weird that we're both waiting for |condition| to return True and also returning the value of |condition|. Can't we assume that if this function doesn't throw an exception that the return of |condition| is True? The condition() function seems inherently unfit to pass back data to the caller, because it can't pass back all values (because some value needs to be reserved for stopping the wait). https://codereview.chromium.org/2451553006/diff/40001/common/py_utils/py_util... common/py_utils/py_utils/__init__.py:97: def WaitFor(condition, timeout): Suggestion: a more common way to solve the problem of wanting to do something less as time progresses is to use something called "exponential backoff" (https://en.wikipedia.org/wiki/Exponential_backoff). The basic idea is that you multiply the poll delay by some factor after every failure. So with an initial wait of 1 second and an exponential factor of 2, the first failure would result in a wait of 1 * 2^0 seconds (1 second), the second failure would result in a wait of 1 * 2^1 seconds (2 seconds), the third failure would result in a wait of 1 * 2^2 seconds (4 seconds), etc. until you hit the timeout. Generally, exponential backoff algorithms allow you to specify an initial wait time, an exponential factor, and a max wait time. I think that all of those would be good things to expose here, because it's going to be hard to anticipate how callers would want to use this code. The advantage of this is that it's customizable depending on whether you're trying to wait for something that's expected to happen on the order of microseconds or on the order of hours, whereas the current implementation doesn't really have that flexibility. It's worth looking quickly at how the "retrying" apache library handles it here, too: https://pypi.python.org/pypi/retrying. We probably don't need anything that complex here, but it's good to at least see. https://codereview.chromium.org/2451553006/diff/40001/common/py_utils/py_util... common/py_utils/py_utils/__init__.py:127: poll_interval = min(max(elapsed_time / 10., MIN_POLL_INTERVAL_IN_SECONDS), Maybe use Telemetry's existing clamp function for this? https://cs.corp.google.com/github/catapult-project/catapult/telemetry/telemet... https://codereview.chromium.org/2451553006/diff/40001/common/py_utils/py_util... File common/py_utils/py_utils/py_utils_unittest.py (right): https://codereview.chromium.org/2451553006/diff/40001/common/py_utils/py_util... common/py_utils/py_utils/py_utils_unittest.py:45: # Use list to pass to inner function because of strange python scoping. Maybe clarify "because of strange Python scoping" to something like "in order to allow a lambda function to modify a variable from the outer scope."?
The CQ bit was checked by rnephew@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2451553006/diff/40001/common/py_utils/py_util... File common/py_utils/py_utils/__init__.py (right): https://codereview.chromium.org/2451553006/diff/40001/common/py_utils/py_util... common/py_utils/py_utils/__init__.py:97: def WaitFor(condition, timeout): On 2016/10/26 17:12:56, charliea wrote: > Suggestion: a more common way to solve the problem of wanting to do something > less as time progresses is to use something called "exponential backoff" > (https://en.wikipedia.org/wiki/Exponential_backoff). > > The basic idea is that you multiply the poll delay by some factor after every > failure. So with an initial wait of 1 second and an exponential factor of 2, the > first failure would result in a wait of 1 * 2^0 seconds (1 second), the second > failure would result in a wait of 1 * 2^1 seconds (2 seconds), the third failure > would result in a wait of 1 * 2^2 seconds (4 seconds), etc. until you hit the > timeout. > > Generally, exponential backoff algorithms allow you to specify an initial wait > time, an exponential factor, and a max wait time. I think that all of those > would be good things to expose here, because it's going to be hard to anticipate > how callers would want to use this code. > > The advantage of this is that it's customizable depending on whether you're > trying to wait for something that's expected to happen on the order of > microseconds or on the order of hours, whereas the current implementation > doesn't really have that flexibility. > > It's worth looking quickly at how the "retrying" apache library handles it here, > too: https://pypi.python.org/pypi/retrying. We probably don't need anything that > complex here, but it's good to at least see. Ned, do you know why the current version was implemented this way instead of will exponential backoff? https://codereview.chromium.org/2451553006/diff/40001/common/py_utils/py_util... common/py_utils/py_utils/__init__.py:97: def WaitFor(condition, timeout): oobe and ios_browser_backend rely on this return. https://codereview.chromium.org/2451553006/diff/40001/common/py_utils/py_util... common/py_utils/py_utils/__init__.py:127: poll_interval = min(max(elapsed_time / 10., MIN_POLL_INTERVAL_IN_SECONDS), On 2016/10/26 17:12:56, charliea wrote: > Maybe use Telemetry's existing clamp function for this? > https://cs.corp.google.com/github/catapult-project/catapult/telemetry/telemet... This part of the repo can't rely on telemetry. https://codereview.chromium.org/2451553006/diff/40001/common/py_utils/py_util... File common/py_utils/py_utils/py_utils_unittest.py (right): https://codereview.chromium.org/2451553006/diff/40001/common/py_utils/py_util... common/py_utils/py_utils/py_utils_unittest.py:45: # Use list to pass to inner function because of strange python scoping. On 2016/10/26 17:12:56, charliea wrote: > Maybe clarify "because of strange Python scoping" to something like "in order to > allow a lambda function to modify a variable from the outer scope."? Done.
Patchset 3 moves telemetry to use the version in py_utils, running tryjob on that to see if there were any mistakes.
On 2016/10/26 17:29:19, rnephew (Reviews Here) wrote: > Patchset 3 moves telemetry to use the version in py_utils, running tryjob on > that to see if there were any mistakes. I think we should only one step at a time. The steps are: 1) Move telemetry's WaitFor method to py_util. 2) Change WaitFor to use exponential back off (if appropriately). Sorry for the misguidance, to unblock your work Randy, we can prioritize step 2 first, which mean provide as WaitFor method in py_utils in this CL. Updating Telemetry to use it should be done in subsequent CL.
Description was changed from ========== [Common] Move WaitFor from telemetry to common/py_utils BUG=catapult:#2955 ========== to ========== [Common] Move WaitFor from telemetry to common/py_utils BUG=catapult:#2955 ==========
nednguyen@google.com changed reviewers: + dtu@chromium.org
On 2016/10/26 17:38:43, nednguyen wrote: > On 2016/10/26 17:29:19, rnephew (Reviews Here) wrote: > > Patchset 3 moves telemetry to use the version in py_utils, running tryjob on > > that to see if there were any mistakes. > > I think we should only one step at a time. The steps are: > 1) Move telemetry's WaitFor method to py_util. > 2) Change WaitFor to use exponential back off (if appropriately). > > Sorry for the misguidance, to unblock your work Randy, we can prioritize step 2 > first, which mean provide as WaitFor method in py_utils in this CL. Updating > Telemetry to use it should be done in subsequent CL. +Dave may know why we don't use expontential back off in Telemetry's WaitFor implementation
https://codereview.chromium.org/2451553006/diff/80001/common/py_utils/py_util... File common/py_utils/py_utils/py_utils_unittest.py (right): https://codereview.chromium.org/2451553006/diff/80001/common/py_utils/py_util... common/py_utils/py_utils/py_utils_unittest.py:29: @mock.patch('time.sleep', mock.Mock) Instead of use mock, can you just use a testing strategy similar to https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... ? I don't like mock very much because they usually draw people to test the implementation instead of test the interface.
> I think we should only one step at a time. The steps are: > 1) Move telemetry's WaitFor method to py_util. > 2) Change WaitFor to use exponential back off (if appropriately). > > Sorry for the misguidance, to unblock your work Randy, we can prioritize step 2 > first, which mean provide as WaitFor method in py_utils in this CL. Updating > Telemetry to use it should be done in subsequent CL. I'm slightly confused, do you want me to undo the changes I made for running py_utils version in telemetry, and redo it in another cl, and instead do exponential backoff in this cl?
https://codereview.chromium.org/2451553006/diff/80001/common/py_utils/py_util... File common/py_utils/py_utils/py_utils_unittest.py (right): https://codereview.chromium.org/2451553006/diff/80001/common/py_utils/py_util... common/py_utils/py_utils/py_utils_unittest.py:29: @mock.patch('time.sleep', mock.Mock) On 2016/10/26 17:42:12, nednguyen wrote: > Instead of use mock, can you just use a testing strategy similar to > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... > ? > > I don't like mock very much because they usually draw people to test the > implementation instead of test the interface. This is only to mock out the sleep method in time, so that when its called it doens't _actually_ sleep for that time.
On 2016/10/26 17:54:22, rnephew (Reviews Here) wrote: > > I think we should only one step at a time. The steps are: > > 1) Move telemetry's WaitFor method to py_util. > > 2) Change WaitFor to use exponential back off (if appropriately). > > > > Sorry for the misguidance, to unblock your work Randy, we can prioritize step > 2 > > first, which mean provide as WaitFor method in py_utils in this CL. Updating > > Telemetry to use it should be done in subsequent CL. > > I'm slightly confused, do you want me to undo the changes I made for running > py_utils version in telemetry, and redo it in another cl, and instead do > exponential backoff in this cl? Correct. Sorry again for the confusion. I thought this was just a straight move.
https://codereview.chromium.org/2451553006/diff/80001/common/py_utils/py_util... File common/py_utils/py_utils/__init__.py (right): https://codereview.chromium.org/2451553006/diff/80001/common/py_utils/py_util... common/py_utils/py_utils/__init__.py:124: logging.info('Continuing to wait %ds for %s. Elapsed: %ds.', timeout, This could be spammy, so I suggest removing this log https://codereview.chromium.org/2451553006/diff/80001/common/py_utils/py_util... File common/py_utils/py_utils/py_utils_unittest.py (right): https://codereview.chromium.org/2451553006/diff/80001/common/py_utils/py_util... common/py_utils/py_utils/py_utils_unittest.py:29: @mock.patch('time.sleep', mock.Mock) On 2016/10/26 17:54:29, rnephew (Reviews Here) wrote: > On 2016/10/26 17:42:12, nednguyen wrote: > > Instead of use mock, can you just use a testing strategy similar to > > > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... > > ? > > > > I don't like mock very much because they usually draw people to test the > > implementation instead of test the interface. > > This is only to mock out the sleep method in time, so that when its called it > doens't _actually_ sleep for that time. If these tests don't invoke the real time.sleep(), that means we don't actually test the time.time() comparison logic?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Catapult Linux Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Li...)
I undid the changes, see comment on the one file left about why its there. https://codereview.chromium.org/2451553006/diff/80001/common/py_utils/py_util... File common/py_utils/py_utils/py_utils_unittest.py (right): https://codereview.chromium.org/2451553006/diff/80001/common/py_utils/py_util... common/py_utils/py_utils/py_utils_unittest.py:29: @mock.patch('time.sleep', mock.Mock) On 2016/10/26 18:00:07, nednguyen wrote: > On 2016/10/26 17:54:29, rnephew (Reviews Here) wrote: > > On 2016/10/26 17:42:12, nednguyen wrote: > > > Instead of use mock, can you just use a testing strategy similar to > > > > > > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... > > > ? > > > > > > I don't like mock very much because they usually draw people to test the > > > implementation instead of test the interface. > > > > This is only to mock out the sleep method in time, so that when its called it > > doens't _actually_ sleep for that time. > > If these tests don't invoke the real time.sleep(), that means we don't actually > test the time.time() comparison logic? The time.time() calls would still yield real results, only the sleep() method is mocked. The logic is still tested, just not much time will have elapsed because of time.sleep() time not actually elapsing. The mocking in the negative case is not required, since it will always have to wait until the end, but I will lower the ammount of time it is waiting to speed the tests up. https://codereview.chromium.org/2451553006/diff/100001/telemetry/telemetry/in... File telemetry/telemetry/internal/results/html_output_formatter.py (right): https://codereview.chromium.org/2451553006/diff/100001/telemetry/telemetry/in... telemetry/telemetry/internal/results/html_output_formatter.py:15: from telemetry.internal.results import html2_output_formatter Uh, to undo my old changes I did a git checkout origin/master -- <file names> I assume these changes originate from that command. I did not make these.
lgtm I am fine with either implement the wait as-is or using exponential back-off https://codereview.chromium.org/2451553006/diff/120001/common/py_utils/py_uti... File common/py_utils/py_utils/__init__.py (right): https://codereview.chromium.org/2451553006/diff/120001/common/py_utils/py_uti... common/py_utils/py_utils/__init__.py:125: GetConditionString(), elapsed_time) pings about remove this log https://codereview.chromium.org/2451553006/diff/120001/telemetry/telemetry/in... File telemetry/telemetry/internal/results/html_output_formatter.py (right): https://codereview.chromium.org/2451553006/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/results/html_output_formatter.py:4: This seems like a rebase error
Killed the logging, I'm going to land it with the current timing implementation, and when david responds I'll change it in another CL if he says we should.
The CQ bit was checked by rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com Link to the patchset: https://codereview.chromium.org/2451553006/#ps160001 (title: "nix logging")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Common] Move WaitFor from telemetry to common/py_utils BUG=catapult:#2955 ========== to ========== [Common] Move WaitFor from telemetry to common/py_utils BUG=catapult:#2955 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
