Implemented multiprocessing and argparse in sct_testing, and other improvements related to Sentry #1872

zougloub · 2018-06-15T18:38:35Z

This PR brings:

sct_testing usage of multiprocessing (Performance improvements for the tests #1539)
- Fix for sct_testing TLS memory usage grows beyond limit #1620 (sct_testing TLS memory usage grows beyond limit) (unless -j 1 is called)
sct_testing usage of argparse (no problem since it's a script used for development only)
sct_download_data to not send Sentry messages (No need to call Sentry if user quits with CTRL+C #1844)
docs
since coveralls was complaining about coverage... generate coverage info from sct_testing

)

perone

I added some comments, I'm marking this PR with approve because I know these changes can take some time and they might be another different PR.

perone · 2018-06-18T15:06:27Z

.travis.yml

+ - bash -c 'echo -ne "[run]\nconcurrency = multiprocessing\nparallel = True\n" > .coveragerc'
+ - COVERAGE_PROCESS_START="$PWD/.coveragerc" COVERAGE_FILE="$PWD/.coverage" PYTHONPATH="$PWD" sct_testing -d 1
+ - coverage combine
+ - ./unit_testing.sh


I wonder if we shouldn't separate these two different tests. Because otherwise, we'll never know what lacks unit-testing or integration tests. However, I'm not sure if coveralls accepts two kinds of distinct testing report.

IMHO:

We'll know that we lack coverage, and improving it can be achieved either by unit or integration tests...

Coveralls is nice and sexy but not as flexible as the bare coverage; we can also get coverage reports locally.

I mentioned having separate coverages because the integration tests are very different than the unit tests. In integration tests, we test with real data, while on unit testing we test with mocked data. But as you mentioned, we can have separate reports locally, so that's not an issue.

perone · 2018-06-18T15:12:19Z

scripts/sct_download_data.py

@@ -214,4 +215,6 @@ def download_data(urls, verbose):

 if __name__ == "__main__":
 sct.init_sct()
- main()
+ res = main()
+ raise SystemExit(res)


This will change the exception KeyboardInterrupt for a SystemExit, it might confuse error reporting, exception handling, etc. Please see the #1658. Let's say someone uses sct_download_data later (as an API), it will silence the KeyboardInterrupt exception in favor of a C-style error reporting of returning a code.
If there is any other way of doing that it would be nice.

The main() function was not exactly behaving as an API-callable function (should have one named download_data() or similar) but rather like a main() function...
As the user could ^C anytime, I just removed the try/catch and the user will get a standard traceback message, as in every other SCT script.

perone · 2018-06-18T15:17:39Z

scripts/sct_testing.py

+ elif jobs == -1:
+ jobs = None
+ else:
+ raise ValueError()


Missing error message in the ValueError exception.

It doesn't matter as the message doesn't get brought to the user's attention (argparse is eating it).

perone · 2018-06-18T15:19:13Z

scripts/sct_testing.py

+ module_testing = importlib.import_module('test_' + fname)
+ # initialize default parameters of function to test
+ param.args = []
+ # param.list_fname_gt = []


Commented code.

I didn't add any commented code, it was just moved from another location.

perone · 2018-06-18T15:19:22Z

scripts/sct_testing.py

+ param_test.default_args = param.args
+ param_test.args = param.args[i]
+ param_test.test_integrity = True
+ # if list_fname_gt is not empty, assign it


Commented code.

perone · 2018-06-18T15:19:39Z

scripts/sct_testing.py

+ param_test = test_function(param_test)
+ except Exception as e:
+ list_status_function.append(1)
+ list_output.append("TODO exception: %s" % e)


TODO message.

Was here before... but yeah, one day.

perone · 2018-06-18T15:26:37Z

scripts/sct_testing.py

+ res = pool.apply_async(process_function, (f, param,))
+ results.append(res)
+
+ for idx_function, f in enumerate(list_functions):


My only concern here is if a message is printed from inside the testing, that will appear here out-of-order, causing confusion on which testing the error message came from because it is executing all of them in parallel.
One solution to that would be capturing and returning the stdout and stderrr inside the process_function(), so you can print these messages here later in order.

Not sure what you mean, the messages are indeed collected and (in case of error) printed in order.

As far as I understood, it is capturing only exceptions but not any other message such as prints, logging, etc. That's my concern.

jcohenadad

i tried launching sct_testing and then did a ctrl+c to quit, but it kept running (after 50 ctrl+c i eventually closed my terminal).

jcohenadad · 2018-06-18T16:39:09Z

for comparison, i tried with different jobs on my 8-cors OSX laptop:

-j 1: 10 min 13 s
-j 2: 5 min 22 s
-j 4: 3 min 25 s
-j 6: 3 min 41 s
-j -1 (default=8): 3 min 10 s

nice job @zougloub 👍

jcohenadad · 2018-06-18T16:54:16Z

scripts/sct_testing.py

+ )
+ parser.add_argument("--jobs", "-j",
+ type=arg_jobs,
+ help="# of simultaneous tests to run (jobs). -1 means # of cores",


i would replace with [...] -1 means # of available cores

Fixed (plus, it's not -1 but 0 now... I can't remember in what state of mind i was when I decided to use -1 instead of zero).

random1717 · 2018-06-18T19:37:36Z

Ubuntu image (2G RAM) -j -1 -> 6m 23s

zougloub · 2018-06-18T19:58:24Z

^C should be handled now.
Note that ^Z + kill %1 can be used in such cases.

random1717 · 2018-06-18T20:18:04Z

@zougloub

Tested, total time = 5m 17s (somehow it reduced!, don't know why)
KeyboardInterrupt raised, but it does not terminate the process (requires ^Z) -> I'm sure it's because of multiprocessing....

zougloub · 2018-06-18T20:26:46Z

The termination is not immediate but it should happen within a few seconds (time for Pool.terminate() to run).

jcohenadad · 2018-06-18T23:02:05Z

i confirm keyboard interrupt works now

Misc. development fixes & improvements Former-commit-id: 7534544

zougloub changed the title ~~Misc~~ Misc. development fixes Jun 15, 2018

zougloub changed the title ~~Misc. development fixes~~ Misc. development fixes & improvements Jun 15, 2018

zougloub force-pushed the cJ-misc branch from 2435d59 to e8c4d2b Compare June 15, 2018 19:18

zougloub requested review from jcohenadad and random1717 June 16, 2018 02:19

zougloub force-pushed the cJ-misc branch 2 times, most recently from fc33c7c to 0a064e9 Compare June 16, 2018 21:05

zougloub added 8 commits June 17, 2018 15:48

docs: sphinx: add some notes about Sentry

4d0aa4e

sct_download_data: don't really error when user cancels downloads (#1844

52ccf17

)

sct_testing: add multiprocessing (#1539)

fdda678

sct_testing: use argparse

ff32ace

Travis: use coverage for sct_testing

7ceb482

Travis: make coverage work with subprocesses

700c483

Travis: be smarter with subprocess coverage

3ce5237

Travis: clean-up

d90e470

zougloub force-pushed the cJ-misc branch from 2676f4a to d90e470 Compare June 17, 2018 19:50

perone approved these changes Jun 18, 2018

View reviewed changes

sct_download_data: remove useless KeyboardInterrupt processing

5dadac3

jcohenadad requested changes Jun 18, 2018

View reviewed changes

jcohenadad reviewed Jun 18, 2018

View reviewed changes

random1717 approved these changes Jun 18, 2018

View reviewed changes

zougloub added 2 commits June 18, 2018 15:46

sct_testing: change documentation (and magic value) of -j argument

f7ba46a

sct_testing: terminate on ^C

7dd80ec

zougloub force-pushed the cJ-misc branch from 2f69afa to 7dd80ec Compare June 18, 2018 19:56

jcohenadad approved these changes Jun 18, 2018

View reviewed changes

zougloub merged commit 7534544 into master Jun 19, 2018

zougloub deleted the cJ-misc branch June 19, 2018 00:35

zougloub mentioned this pull request Jul 6, 2018

No need to call Sentry if user quits with CTRL+C #1844

Closed

jcohenadad added the tests context: unit, integration, or functional tests label Jul 7, 2018

jcohenadad added this to the v3.2.2 milestone Jul 7, 2018

jcohenadad changed the title ~~Misc. development fixes & improvements~~ sct_testing: Implemented multiprocessing and argparse, and other improvements related to Sentry Jul 7, 2018

jcohenadad changed the title ~~sct_testing: Implemented multiprocessing and argparse, and other improvements related to Sentry~~ Implemented multiprocessing and argparse in sct_testing, and other improvements related to Sentry Jul 7, 2018

jcohenadad mentioned this pull request Dec 17, 2019

sct_testing TLS memory usage grows beyond limit #1620

Closed

jcohenadad pushed a commit that referenced this pull request Dec 18, 2019

Merge pull request #1872 from neuropoly/cJ-misc

59a5cd4

Misc. development fixes & improvements Former-commit-id: 7534544

joshuacwnewton mentioned this pull request Oct 14, 2020

Performance improvements for the tests #1539

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented multiprocessing and argparse in sct_testing, and other improvements related to Sentry #1872

Implemented multiprocessing and argparse in sct_testing, and other improvements related to Sentry #1872

zougloub commented Jun 15, 2018 •

edited

Loading

perone left a comment

perone Jun 18, 2018

zougloub Jun 18, 2018

perone Jun 18, 2018

perone Jun 18, 2018

zougloub Jun 18, 2018

perone Jun 18, 2018

zougloub Jun 18, 2018

perone Jun 18, 2018

zougloub Jun 18, 2018

perone Jun 18, 2018

perone Jun 18, 2018

zougloub Jun 18, 2018

perone Jun 18, 2018

zougloub Jun 18, 2018

perone Jun 18, 2018

jcohenadad left a comment •

edited

Loading

jcohenadad commented Jun 18, 2018 •

edited

Loading

jcohenadad Jun 18, 2018

zougloub Jun 18, 2018

random1717 commented Jun 18, 2018

zougloub commented Jun 18, 2018

random1717 commented Jun 18, 2018

zougloub commented Jun 18, 2018

jcohenadad commented Jun 18, 2018

Implemented multiprocessing and argparse in sct_testing, and other improvements related to Sentry #1872

Implemented multiprocessing and argparse in sct_testing, and other improvements related to Sentry #1872

Conversation

zougloub commented Jun 15, 2018 • edited Loading

perone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcohenadad left a comment • edited Loading

Choose a reason for hiding this comment

jcohenadad commented Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

random1717 commented Jun 18, 2018

zougloub commented Jun 18, 2018

random1717 commented Jun 18, 2018

zougloub commented Jun 18, 2018

jcohenadad commented Jun 18, 2018

zougloub commented Jun 15, 2018 •

edited

Loading

jcohenadad left a comment •

edited

Loading

jcohenadad commented Jun 18, 2018 •

edited

Loading