Rm/2426 #2517
Open
Rm/2426 #2517
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There is a bug souffle-lang#2426, which causes an assertion in the SelingerProfileSipsMetric::getReordering. Rther than fixing the problem I decided to work-around it for the time being, until we have a proper fix. We can work around this problem in one of two ways: 1. don't assert and just don't reorder atoms (this should be fine) 2. don't create the metric in the first place, if there is no profile I decided to use souffle-lang#2 as that seems to be the safer option. I added code for a warning, but disabled it because it is incompatible with the ctest suite.
We should print a warning about --auto-schedule not being enabled. If we do that we also need to change the unit test setups for scheduler tests.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2517 +/- ##
==========================================
- Coverage 81.12% 81.11% -0.01%
==========================================
Files 492 492
Lines 33361 33362 +1
==========================================
- Hits 27063 27062 -1
- Misses 6298 6300 +2
|
quentin
reviewed
Nov 18, 2024
Comment on lines
+316
to
+321
| // TODO: enable this, but if we do the scheduler tests won't run as they do now | ||
| #if 0 | ||
| std::cerr << "WARNING: `--auto-schedule` cannot be used due to missing scheduler stats; falling back " | ||
| "to heuristic '" | ||
| << heuristic << "'" << ::std::endl; | ||
| #endif |
There was a problem hiding this comment.
I think we should print the warning, but I'm not too familiar with CMAKE to make the updates to the test scripts.
There was a problem hiding this comment.
Do you mind merging as is?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #2426 shows a problem (crash) when using the auto-scheduler on valid input. This MR is an attempt to circumvent the issue by not using the auto-scheduler if there is no good profile and falling back to a heuristic.
I realize that this is not ideal, but I would prefer using a heuristic to crashing. This fix also gives us some more time to address the underlying bug. If you think the underlying bug is easy to fix, I can take a stab at it, but since it's been open for 1 year, it's probably harder to fix.
There is code for a warning message when there is a problem with the auto-scheduler, but I could not figure out how to integrate that into the ctest suite. The problem is because the warning causes the two expected err outputs to be different between the two runs and the test isn't set up to support that.