Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix recover and pedantic behavior #13579

Merged
merged 6 commits into from
Jun 25, 2019
Merged

Conversation

milljm
Copy link
Member

@milljm milljm commented Jun 13, 2019

Fix recover and pedantic skipped dependencies.
Optimize pedantic operations.

Closes #13578

@milljm milljm requested a review from permcody June 13, 2019 22:11
@milljm
Copy link
Member Author

milljm commented Jun 13, 2019

A little rushed in the end. But this seems to be working.

exit_code = status.race.code

else:
print("There are no race conditions.")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

woops. This got deleted by accident

self._doMakeSerializeDependencies()
for job in self.__job_dag.topological_sort():
job.addDownsteamNodes(self.__job_dag.all_downstreams(job))
job.addUpsteamNodes(self.__job_dag.predecessors(job))
Copy link
Member Author

@milljm milljm Jun 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure this is necessary anymore. Now that the DAG is as it was, we can, should be able to call the DAG's predecessors/all_downstreams.

@moosebuild
Copy link
Contributor

moosebuild commented Jun 13, 2019

Job Documentation on 03e210d wanted to post the following:

View the site here

This comment will be updated on new commits.

@@ -33,22 +33,6 @@ def __cacheGraph(self):
self.__cached_graph = self.clone()
return self.__cached_graph

# Added by the MOOSE group
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to suggest that we don't remove this method. I'm still not convinced that we don't need it if we are doing things right. When I have some time, I'd like to sit down and really go through a few scenarios of the DAG. I've thought about this a lot and don't see why we don't need this method. Just leave it in for now in case we decide we'll want it later.

@milljm
Copy link
Member Author

milljm commented Jun 14, 2019

There is an issue relating to the print output when multiple race conditions exist. I'm looking into it. Please do not merge.

@permcody
Copy link
Member

This still doesn't work - the problem here now is that the test harness prints that it's rerunning Part1 of a two part test but it doesn't actually rerun it. This is bad because the data created in Part1 might be changing (if the developer is working on something) but since the TestHarness isn't actually doing anything now, it won't rerun Part1 so Part2 will always fail.

To test:

  1. Go to simple_transient_diffusion and change one of the boundary condition values (e.g. 1 -> 2)
  2. ./run_tests -j 24 --re=simple --recover # You should see a failure
  3. Change the value in Removed #1 back
  4. ./run_tests -j 24 --re=simple --recover --failed-tests -v

You will note that it prints the Part1 and Part2 of the failed test, which is good, but it doesn't actually run the command. It just reports OK. I verified that current HEAD reruns the command as expected.

Copy link
Member

@permcody permcody left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment

@milljm
Copy link
Member Author

milljm commented Jun 25, 2019

That is sorta impossible:

if status == tester.success and self._hasDownStreamsWithFailures(job):

I specifically designed it not to re-run the part 1 test. If you actually want part one to execute, thats an easy fix.

         if status == tester.success and self._hasDownStreamsWithFailures(job):
-            tester.setStatus(tester.success)
-            # Do we care? I figure, we should at least mention something. This job is not actually going to re-execute an app.
-            tester.addCaveats('previous results: {}'.format(status.status))
-            job.setStatus(job.finished)
+            tester.addCaveats('re-running')
+            return

milljm and others added 6 commits June 25, 2019 11:52
Fix recover and pedantic skipped dependencies.
Optimize pedantic operations.

Closes idaholab#13578
Continue to reduce code necessary to perform pedantic testing

Refs idaholab#13578
The following methods are no longer necessary.
When running --pedantic, and no issues found, we should say so.
Re-adding serialized DAG routines.
Remove un-used upstream/downstream methods.

Refs idaholab#13578
File matches need to be updated, not clobbered.
When using --failed-tests and --recover, make the part 1 test actually re-execute.
@permcody
Copy link
Member

You do realize you said exactly opposite things, right? I'm going to go with the "easy fix" not "sorta impossible" comment

@milljm milljm deleted the recover-pedantic-13578 branch March 20, 2020 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix --recover and --pedantic
3 participants