[wip] [feat] Add support for running interdependent tests with the serial execution policy #944

ChristopherBignamini · 2019-09-18T15:08:33Z

No description provided.

pep8speaks · 2019-09-18T15:08:38Z

Hello @ChristopherBignamini, Thank you for updating!

Cheers! There are no PEP8 issues in this Pull Request!Do see the ReFrame Coding Style Guide

Comment last updated at 2019-10-25 14:37:28 UTC

vkarak · 2019-09-19T09:59:21Z

@ChristopherBignamini Unit tests fail. Can you fix them?

…aph type initialization

ChristopherBignamini · 2019-09-20T15:53:57Z

I have a two tests that fail (randomly) with Python 3.5

test_kbd_interrupt_in_wait_with_limited_concurrency and test_kbd_interrupt_in_setup_with_concurrency

In both cases there are no dependencies but I am calling the toposort method on the test list independently of the presence of dependencies. The resulting test list is reordered wrt the original list specified in the unit test and this cause, sometimes, the failure of the test. I could check if there is any dependency and if not skip the toposort step. Alternatively, I can think of a possible fix for the unit tests.

vkarak · 2019-09-22T09:38:12Z

reframe/frontend/cli.py

            exec_policy.sched_nodelist = options.nodelist
            exec_policy.sched_exclude_nodelist = options.exclude_nodes
            exec_policy.sched_options = options.job_options
+


Remove this line.

vkarak · 2019-09-22T09:39:06Z

reframe/frontend/dependency.py

+    # TODO: create unit test
+    deps_count = {}
+    for test, deps in graph.items():
+        try:


Why this try/except is needed here?

I need all the checks that will be executed in a reframe run in the ref_count: let's say that I've completed the task for check C, I need to know if I can delete the corresponding output and I do that when ref_count[c] goes to 0 as I do with checks C dependencies. For this reason I need to include C in the list and I'm doing it in the same way of its dependencies.

No, this I understand. My point here was about whether the double try/except clause is actually needed.

Also I don't understand the first += 1? Sth like the following should be enough I guess:

for test, deps in graph.items(): ref_count.setdefault(test, 0) for d in deps: try: ref_count[test] += 1 except KeyError: ref_count[test] = 0

Yes, you are right, I don't need the +1.
By the way, I see that you are using the test variable as key in the inner loop instead of using dep, but currently the content of ref_count[c], for a given check c, is the number of checks depending on c. Should I change the ref_count data meaning?

vkarak · 2019-09-22T09:39:30Z

reframe/frontend/dependency.py

+            deps_count[test] += 1
+        except KeyError:
+            deps_count[test] = 0
+        for dep in deps:


Suggested change

for dep in deps:

for adj in deps:

vkarak · 2019-09-22T09:40:01Z

reframe/frontend/dependency.py

+
+    # TODO: create unit test
+    deps_count = {}
+    for test, deps in graph.items():


Suggested change

for test, deps in graph.items():

for node, deps in graph.items():

vkarak · 2019-09-22T09:43:15Z

unittests/test_cli.py

        self.assertEqual(0, returncode)
+
+    def test_dependency_cli(self):
+        self.checkpath = ['unittests/resources/checks/frontend_checks.py']


I wouldn't put these tests in the frontend_checks.py but rather in a different subdirectory next to the resources/checks. You wouldn't need then to update the loader unit tests.

You can put them inside checks_unlisted/.

Do I have to put everything in /unittests/resources/checks_unlisted/? I mean the main test file and the single tests? There is a dependencies subfolder, should I put everything in there?

vkarak · 2019-09-22T20:41:53Z

reframe/frontend/executors/__init__.py

        self.stats = None

+        # Check dependencies data
+        self.dependency_tree = collections.OrderedDict()


I don't understand why you initialise it like that. Check my comment above on how to redesign it.

vkarak · 2019-09-22T20:43:14Z

reframe/frontend/executors/policies.py

+                            dependency_clean_up_task.cleanup(
+                                not self.keep_stage_files,
+                                False)
+                            break


Why all this has to be in the finally?

I need to be sure that the cleanup step is always executed, also in case of exceptions.

Why it should be that? If a test fails, its cleanup phase is not executed by just throwing the exception. Also I think that this part might be simplified if you make the serial policy also a TaskEventListener so that it gets notified when a task finishes. Then you can adjust the reference count in a cleaner way.

vkarak · 2019-09-22T20:52:43Z

reframe/frontend/executors/__init__.py

+        dependency.print_deps(dependency_graph)
+        testcases = dependency.toposort(dependency_graph)
+        self._policy.dependency_tree = dependency_graph
+        self._policy.dependency_count = dependency.create_deps_count(


I don't like very much the dependency_count as a name. I would call it ref_count and have a comment explaining it. In practice, you are doing a reference counting.

vkarak · 2019-09-22T21:13:16Z

reframe/frontend/executors/policies.py

+                self.dependency_count[dep] -= 1
+                if self.dependency_count[dep] == 0:
+                    # Check if dep has failed before cleaning
+                    for t in self.stats.tasks():


It would be better to associate the reference counter to the RegressionTask so that you avoid this for loop here and the use of stats.

vkarak · 2019-09-22T21:15:12Z

reframe/frontend/executors/policies.py

+                    # Check if dep has failed before cleaning
+                    for t in self.stats.tasks():
+                        if t.testcase == dep and t.failed is False:
+                            dependency_clean_up_task = RegressionTask(dep)


Why do you create a new "task" here? I'd just call the deferred cleanup() on the original one.

Also cleanup may throw, so it has to be treated correctly. That's why I don't think putting this loop in the finally is correct.

vkarak · 2019-10-04T09:48:34Z

@ChristopherBignamini Can you fix the conflicts with the master?

ekouts · 2019-10-25T11:40:24Z

unittests/resources/checks_unlisted/cleanup_checks.py

+
+
+@rfm.simple_test
+class DependencyT0(rfm.RunOnlyRegressionTest):


Isn't self.sourcesdir = None missing from these tests?

vkarak · 2019-10-27T23:59:52Z

Closing this one. Replaced by #1015.

ChristopherBignamini added 7 commits August 27, 2019 18:06

WIP Dependency unit testing development

c6aa6ca

Merge branch 'master' into inter_depentent_check_unit_test

badd939

Merge branch 'master' into inter_depentent_check_unit_test

e7bb5d3

test cleanup procedure implemented with dependencies

9fbfb49

Merge branch 'master' into inter_depentent_check_unit_test

548082c

Merge with master

60d2156

Bug fixed

bca4959

ChristopherBignamini requested a review from vkarak September 18, 2019 15:08

ChristopherBignamini changed the title ~~[WIP] Test dependency for serial execution policy~~ [wip] Test dependency for serial execution policy Sep 18, 2019

Coding style fixes

4a9cb19

vkarak requested a review from teojgo September 19, 2019 09:58

vkarak assigned ChristopherBignamini Sep 19, 2019

vkarak added enhancement prio: normal labels Sep 19, 2019

vkarak added this to the ReFrame sprint 2019w38 milestone Sep 19, 2019

ChristopherBignamini added 2 commits September 19, 2019 17:14

Updated number of loaded checks in unit tests and fixed dependency gr…

ba6b693

…aph type initialization

Dependency graph building moved into Runner class

ef9acf6

vkarak changed the title ~~[wip] Test dependency for serial execution policy~~ [wip] [feat] Test dependency for serial execution policy Sep 20, 2019

vkarak changed the title ~~[wip] [feat] Test dependency for serial execution policy~~ [wip] [feat] Add support for running interdependent tests with the serial execution policy Sep 21, 2019

vkarak requested changes Sep 22, 2019

View reviewed changes

vkarak modified the milestones: ReFrame sprint 2019w38, Upcoming sprint Sep 30, 2019

Code cleaning

db06b16

ChristopherBignamini added 3 commits October 4, 2019 12:26

Cleanup with dependency tests moved

268f9e6

Code cleaning

023b283

Dependecies cleanup tests moved to new location

75ae31d

ChristopherBignamini added 4 commits October 4, 2019 15:08

Code cleaning

279757a

Code cleaning

b3f726b

Merge branch 'master' into inter_depentent_check_unit_test

fcde831

Coding style fix

4346353

vkarak removed this from the ReFrame Sprint 2019w41 milestone Oct 23, 2019

ChristopherBignamini added 3 commits October 24, 2019 14:11

Merge branch 'master' into inter_depentent_check_unit_test

5d9590c

Merge branch 'master' into inter_depentent_check_unit_test

b276f50

Coding style fixes

9ee4fb8

ekouts reviewed Oct 25, 2019

View reviewed changes

Exception handling fix in cleanup procedure.

f181c8b

vkarak mentioned this pull request Oct 27, 2019

[feat] Add support for test dependencies in serial execution policy #1015

Merged

vkarak closed this Oct 27, 2019

	for test, deps in graph.items():
	for node, deps in graph.items():



		@rfm.simple_test
		class DependencyT0(rfm.RunOnlyRegressionTest):

[wip] [feat] Add support for running interdependent tests with the serial execution policy #944

[wip] [feat] Add support for running interdependent tests with the serial execution policy #944

Uh oh!

Conversation

ChristopherBignamini commented Sep 18, 2019

Uh oh!

pep8speaks commented Sep 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2019-10-25 14:37:28 UTC

Uh oh!

vkarak commented Sep 19, 2019

Uh oh!

ChristopherBignamini commented Sep 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vkarak commented Oct 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vkarak commented Oct 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pep8speaks commented Sep 18, 2019 •

edited

Loading