[PY3] unit test fixed for Aligenmnet #28286
Conversation
|
please test |
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28286/12470
|
|
The tests are being triggered in jenkins. |
|
A new Pull Request was created by @smuzaffar (Malik Shahzad Muzaffar) for master. It involves the following packages: Alignment/CommonAlignment @christopheralanwest, @tocheng, @cmsbuild, @franzoni, @tlampen, @pohsun can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
| @@ -363,14 +366,14 @@ def _request_dataset_information(self): | |||
| for d in self._datasets: print_msg("\t"+d) | |||
| print_msg("This may take a while...") | |||
|
|
|||
| result = pool.map_async(get_events_per_dataset, self._datasets).get(sys.maxsize) | |||
| result = pool.map_async(get_events_per_dataset, self._datasets).get(3600) | |||
There was a problem hiding this comment.
@smuzaffar sys.maxint has gone in python3, but isn't sys.maxsize still available, according to https://docs.python.org/3/library/sys.html ? What is the rationale behind this hardcoded value?
There was a problem hiding this comment.
ints are gone in python3. so maxsize is available but does not give the same answer as in python2.
The error is
OverflowError: timestamp too large to convert to C _PyTime_t (which instead is a 64-bit int in C)
but perhaps 2147483647 would conserve the old behavior?
There was a problem hiding this comment.
@fabiocos , sys.maxint and sys.maxsize are used here to set a timeout (i.e. 9223372036854775807 sec). There is no need to set such long timeouts.
There was a problem hiding this comment.
@davidlange6 according to the documentation sys.maxsize is now a variable of type Py_ssize_t so not what is desired here, I agree
There was a problem hiding this comment.
@smuzaffar this argument is more related to the functionality of the unit test I would say, waiting for an infinite time is not what is desired
|
@christopheralanwest @tlampen @pohsun the changes look technical, please check, I would like to integrate this after pre11 is built |
|
+1 |
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
|
if 3600 is 3600 seconds as a max limit for this command, then it seems sufficiently large to allow a set of das queries to run.
… On Oct 29, 2019, at 11:50 AM, Fabio Cossutti ***@***.***> wrote:
@fabiocos commented on this pull request.
In Alignment/CommonAlignment/scripts/tkal_create_file_lists.py:
> @@ -363,14 +366,14 @@ def _request_dataset_information(self):
for d in self._datasets: print_msg("\t"+d)
print_msg("This may take a while...")
- result = pool.map_async(get_events_per_dataset, self._datasets).get(sys.maxsize)
+ result = pool.map_async(get_events_per_dataset, self._datasets).get(3600)
@smuzaffar this argument is more related to the functionality of the unit test I would say, waiting for an infinite time is not what is desired
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
yes it is in seconds. I leave it to alca developers to set it to some reasonable time. For IBs, we kill unit test after 30 mins anyway :-) |
This file (which is tested in a unit test) is also used for the actual aggregation of the data for running the alignment. Maybe waiting for an infinite amount of time is not OK for a unit test, but on the other hand I am not sure if the script is supposed to run for > 1h in real life applications. @adewit @connorpa. |
|
presumably one can set this to a big enough number for all usecases - however i guess cms actually doesn't want scripts running hours worth of das queries either....
… On Oct 29, 2019, at 1:49 PM, Marco Musich ***@***.***> wrote:
yes it is in seconds. I leave it to alca developers to set it to some reasonable time. For IBs, we kill unit test after 30 mins anyway :-)
This file (which is tested in a unit test) is also used for the actual aggregation of the data for running the alignment. Maybe waiting for an infinite amount of time is not OK for a unit test, but on the other hand I am not sure if the script is supposed to run for > 1h in real life applications. @adewit @connorpa.
Maybe that parameter can be made configurable and set to 1h for the unit test.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
@mmusich @connorpa @adewit @christopheralanwest can we clarify what is the real life need for this timeout? For the unit test purpose the solution by @smuzaffar is clearly ok, in case we have different use cases we should clarify a meaningful value, possibly before integrating this PR |
The feedback I got in the meanwhile is that it usually takes few minutes to run, so 1h timeout sounds about reasonable also for the offline use-case. |
|
@mmusich thank you, I will move forward with the integration of this PR in next IB. In case we may update it further according to possible different needs |
|
+1 |
These changes are needed to fix unit tests for Alignment for python3