Iron-out edge cases for library use-case, adding extensive real-world test assertions #2309

lucksus · 2024-01-29T17:30:13Z

Additions

result assertions for the lib_machine integration tests (which are a log of our ad4m integration tests, copying how ad4m is driving the scryer machine during our test runs).
two regular test cases that are minimal examples of the first failing result assertion from the integration tests.

Context

After merging #1880 we tried to jump to upstream scryer commits/versions but were forced to stay at an old commit that was still using our custom toplevel. Using the fully Rust-based version that got merged we couldn't get our full integration tests to pass.

After double checking our code, I went ahead and completed the integration tests here with the results that our test code expects to get from scryer. (that has not really changed from our old SWI integration over the toplevel based scryer lib_machine).

1. problems with `discontiguous`

Interpreting the new test results, I think it's clear that the problem only occurs when declaring a predicate with discontiguous. In the new test dont_return_partial_matches we load the program:

:- discontiguous(property_resolve/2).
subject_class("Todo", c).

which is all we need from our integration tests to show the faulty behaviour when running the query:

subject_class("Todo", C), property_resolve(C, "isLiked").

which (wrongly) yields a match for C: "c".

Changing the order of the predicates in the query gives a different result:

property_resolve(C, "isLiked"), subject_class("Todo", C).

=> false

The second new test case dont_return_partial_matches_without_discountiguous shows that a similar case works fine when discontiguous is not used.

fixed in: 7de693e

2. Assertion 50 leaves `run_query` loop

Assertion 50 fails with only one of 3 expected matches.
Debugging through this case shows a strange behaviour: after the first execution of the loop in run_query (ln. 111) which adds the first match, the loop is exited at the end, not calling dispatch_loop() again. This seems to happen without hitting a break.

Resembling this assertion in its own test case by running all previous consults and then running the query and assertion does not yield this strange behaviour.

Seems like the history of all the queries happening before in the integration test are relevant to replicate this.

Debugger jumps to the first line of src/lib.rs when exiting the loop (and then continues after the loop). I have tried increasing and removing the #![recursion_limit = "4112"] there to no avail.

Results are logs of what we get with old toplevel-based version of lib_machine. These are also congruent with what our tests logged out based on SWI.

lucksus · 2024-01-31T14:26:11Z

@mthom after fixing the ordering of the first assertions, I'm now stuck with this:
(also added to the PR description)

Assertion 50 fails with only one of 3 expected matches.
Debugging through this case shows a strange behaviour: after the first execution of the loop in run_query (ln. 111) which adds the first match, the loop is exited at the end, not calling dispatch_loop() again. This seems to happen without hitting a break.

Resembling this assertion in its own test case by running all previous consults and then running the query and assertion does not yield this strange behaviour.

Seems like the history of all the queries happening before in the integration test are relevant to replicate this.

Debugger jumps to the first line of src/lib.rs when exiting the loop (and then continues after the loop). I have tried increasing and removing the #![recursion_limit = "4112"] there to no avail.

Skgland · 2024-01-31T17:29:51Z

#![recursion_limit] is an attribute of the rust compiler, it tells the compiler how deep it should be allowed to recurse before failing compilation, so this shouldn't effect the debugging of the program.

Though if removing it doesn't break the build, then it appears to have become superfluous.

mthom · 2024-02-01T00:34:01Z

I fixed the Assertion 50 error and in the process I believe I exposed a redundant fact in the integration test text file causing a solution to be reported twice instead of once as expected. Please check its diff to see it. Now Assertion #57 fails because the contents of the solution are out of order again.

lucksus · 2024-02-01T12:47:40Z

Alright! We got our ad4m integration test suite passing with commit 53028a9. I've fixed all the orderings in the integration assertions and have all scryer tests pass locally :)

lucksus · 2024-02-01T13:16:56Z

...almost.
Another test suite on our side fails because scryer panics when a query contains a nonexistent predicate, but only if other predicates where defined. I've just added a new test showing this. With the consult call commented-out, this test would pass.

Skgland · 2024-02-01T13:21:31Z

As these are supposed to be integration test, based on https://doc.rust-lang.org/cargo/guide/tests.html they should go into the tests/ folder.
Also, that way they can't accidentally use library internals.

lucksus · 2024-02-01T15:33:21Z

As these are supposed to be integration test, based on https://doc.rust-lang.org/cargo/guide/tests.html they should go into the tests/ folder. Also, that way they can't accidentally use library internals.

Sorry for the confusion, I called the test "integration" because these are assertions coming from our (ad4m) integration tests where we run scryer as library integrated in ad4m. The tests in lib_machine.rs are testing run_query() located in the same file.

lucksus · 2024-02-01T16:05:41Z

Huh, interesting that the last mentioned problem does not occur on wasm and i686 architectures, as seen in CI runs... (but all other archs)

lucksus · 2024-02-01T17:15:34Z

🥳

Skgland · 2024-02-06T09:17:41Z

src/rcu.rs

@@ -22,7 +22,7 @@ thread_local! {
    // odd value means the current thread is about to access the active_epoch of an Rcu
    // a thread has a single epoch counter for all Rcu it accesses,
    // as a thread can only access one Rcu at a time
-    static THREAD_EPOCH_COUNTER: OnceCell<Arc<AtomicU8>> = OnceCell::new();
+    static THREAD_EPOCH_COUNTER: OnceCell<Arc<AtomicU8>> = const { OnceCell::new() };


Ah, I just noticed in the thread_local! docs:

This macro supports a special const {} syntax that can be used
when the initialization expression can be evaluated as a constant.
This can enable a more efficient thread local implementation that
can avoid lazy initialization.

So, I think this change makes sense.

src/machine/parsed_results.rs

Skgland · 2024-02-05T14:00:50Z

src/machine/lib_machine.rs

@@ -524,4 +531,82 @@ mod tests {
            ),]))
        );
    }
+
+    #[test]


Based on the PR description these tests are the result of attempting to use scryer-prolog as a library. As such I would expect these to be integration test that do just that.
On a quick glance these tests also appear to only use the public accessible interface of scryer as a library, so I think these could be just moved to be integration tests in tests/.

Based on you comment at #2309 (comment) you appear to disagree, could you explain why?

I would say this PR (and the ping-pong between me and Mark) demonstrates that the tests here are, first and foremost, covering the function run_query() in this same file. So I would suggest to keep them here because the context of the test be located near run_query() provides import meaning, independent of the visibility of that function.

That said, I'm happy to move it over to tests/ if that's where you want it to be :)
(just explaining my reasoning)

You are talking about all tests in this file?

I have invited you to our forked repo so you can push changes to this branch if you like, @Skgland!

I think I had some misconception/misunderstanding while writing that comment.

While I most of these would make sense as integration tests, as they appear to only use the public interface, I think most are small enough to be fine as is, especially as most already existed here from before this PR.

Only integration_test due to the now huge lib_integration_test_commands.txt (12k+ lines) appears too large.
Maybe the txt file could be split into multiple smaller txt files, so that the files can still be viewed here on Github?
Not sure how intertwined everything in there is.

integration_test could then be split into multiple test functions including the individual txt files which would then call a helper function that is basically the current integration_test but taking code as an argument.

I think it's adequate to question if this test should be here at all. It was a pragmatic way for us to delineate where the problems are stemming from as we did some refactoring in our project at the same time we switched from SWI to Scryer, but had our integration tests from before.

Running this big txt file here is a bit opaque, which is also why I tried to extract the failing assertions into the other more understandable test cases. But it did already uncover a problem that you only see if you have multiple consecutive calls to consult followed by queries. This is actually close the reason that made us add this test mechanism in the first place: we saw scryer slowing down query by query when used in our ad4m integration tests (that's why we had the previous version without assertions on the results - it was just making sure Scryer would make it to the end). So I would argue against splitting it up.

But yeah, that does make it feel more like an integration test.
I'll leave it completely up to you guys to decide if you want to keep this test at all, and if so where it should stay.

I think its not worth blocking the whole PR about this, as it can always be moved later.

Co-authored-by: Bennet Bleßmann <bennet.blessmann+github@googlemail.com>

lucksus · 2024-02-09T12:10:20Z

The style check workflow uses nightly Rust, which even fails the regular tests.

… in CI

Skgland · 2024-02-09T12:28:56Z

The style check workflow uses nightly Rust, which even fails the regular tests.

The problem is apparently that the stdsimd feature was split into sub-features and removed.
This breaks the build of the ahash dependency on nightly.
Something (ahash?) appears to automatically enables nightly features when the use of nightly is detected, which can break the build (and did here) if nightly features change.

Skgland · 2024-02-09T12:32:04Z

I think updating ahash from 0.8.6 to 0.8.7 might be sufficient, unless that reveals further broken dependencies.

lucksus · 2024-02-09T12:33:39Z

Yeah, I mean these things can happen "over night" ;) so my latest commit is a suggestion to not use nightly in CI if not absolutely necessary.. the wasm target build surprisingly works despite nightly. I guess ahash is not build there.. (?)

Skgland · 2024-02-09T12:36:34Z

Yeah, I mean these things can happen "over night" ;) so my latest commit is a suggestion to not use nightly in CI if not absolutely necessary.. the wasm target build surprisingly works despite nightly. I guess ahash is not build there.. (?)

Yeah, I think that is good for the style and report CI.

Maybe the nightly CIs can be marked continue-on-error instead of disabling them.

lucksus · 2024-02-09T12:51:12Z

Ah, I see. So the build and test actually failed for the wasm target (https://github.com/mthom/scryer-prolog/actions/runs/7843777400/job/21404819971) but because of continue-on-error it showed up as green tick anyways... hm, what is the benefit of running these jobs in CI if they won't trigger a failure?

Skgland · 2024-02-09T17:56:01Z

Right, I forgot that GitHub Actions doesn't differentiate between success and failed but allowed to fail. This way the original failing CI would be better as the error is at least visible.

- fix nightly build - not bumping to latest aka. 0.8.8 as that has a msrv of 1.72.0 and we are only at 1.70.0

- currently `continue-on-error` is not shown in a usefull way on failiure see <https://github.com/orgs/community/discussions/15452>

Skgland · 2024-02-16T21:25:35Z

I bumped ahash to 0.8.7 to fix nightly and undid the continue-on-error changes to resolve #2334 (comment)

Skgland · 2024-02-16T21:36:20Z

Looks like the transitive wait-timeout dependency isn't compatible with wasm.
Though I don't know why it fails in the build job rather than the test job,
as it should only be a dev-dependency:

> cargo tree -i -p wait-timeout
wait-timeout v0.2.0
├── assert_cmd v1.0.8
│   [dev-dependencies]
│   └── scryer-prolog v0.9.3 (/mnt/c/Users/Bennet/Git/scryer-prolog-1)
└── snapbox v0.4.15
    └── trycmd v0.14.19
        [dev-dependencies]
        └── scryer-prolog v0.9.3 (/mnt/c/Users/Bennet/Git/scryer-prolog-1)

triska · 2024-02-16T21:43:05Z

Is a failing nightly build even worth delaying PRs? It seems that issues with nightly may also be resolved by completely unrelated changes in other crates that will be made according to their own schedule.

Skgland · 2024-02-16T22:09:22Z

Is a failing nightly build even worth delaying PRs? It seems that issues with nightly may also be resolved by completely unrelated changes in other crates that will be made according to their own schedule.

The only one that's still failing is wasm32 and the test failure already present on master just ignored.
It appears to be mostly tests that are failing to build as some dev-dependencies don't support wasm32.
I am currently trying to disabling those dev-dependencies for wasm32 and #[cfg]ing out the tests that use them

lucksus · 2024-02-26T13:40:51Z

What is needed for this PR to get merged? Please let me know if there is something left I can do.

mthom · 2024-02-28T01:22:39Z

Get the nightly tests to pass, I suppose? I'm not sure how important they are ultimately.

aarroyoc · 2024-02-28T18:22:37Z

Get the nightly tests to pass, I suppose?

They already pass

I'm not sure how important they are ultimately.

It's a good question. Rust should never break compatibility but this rule doesn't apply for nightly, so sometimes it can fail due to mistakes in the Rust side.

lucksus and others added 6 commits January 26, 2024 17:18

Add expected results to integration test

cb01409

Results are logs of what we get with old toplevel-based version of lib_machine. These are also congruent with what our tests logged out based on SWI.

Merge branch 'master' into library-use-case

7e973a6

Minimal reproduction of faulty behaviour seen in integration tests

2e728c7

Add more test cases to differentiate usage of discontiguous

9aacfff

fmt

7bb9c00

check for True or False Query Resolution unconditionally

7de693e

mthom mentioned this pull request Jan 29, 2024

Check for query failure before emitting matches in lib_machine.rs (WIP) #2310

Closed

lucksus added 2 commits January 30, 2024 12:54

Adjust some first result orderings in integration assertions

48b2379

Extract failing assertion as single test case

bc02fb3

lucksus changed the title ~~Additional tests for lib_machine showing problems with discontiguous~~ Iron-out edge cases for library use-case, adding extensive integration tests Jan 31, 2024

mthom added 2 commits January 31, 2024 17:30

index stub choice point correctly

33fc2ed

fix style errors

53028a9

lucksus added 3 commits February 1, 2024 12:59

More ordering adjustments

f35d628

Remove test with long program literals, not needed

e1b0ba4

Fix all orderings in integration assertions

8a6ea29

lucksus marked this pull request as ready for review February 1, 2024 12:47

lucksus added 3 commits February 1, 2024 13:51

clippy

a0e598b

fmt

6586657

Test show problem with nonexistent predicate

06f198b

lucksus changed the title ~~Iron-out edge cases for library use-case, adding extensive integration tests~~ Iron-out edge cases for library use-case, adding extensive real-world test assertions Feb 1, 2024

record stub choice point as block

cbb422f

Skgland reviewed Feb 6, 2024

View reviewed changes

Update src/machine/parsed_results.rs

b43f097

Co-authored-by: Bennet Bleßmann <bennet.blessmann+github@googlemail.com>

mthom force-pushed the library-use-case branch from 32a2ff3 to b43f097 Compare February 9, 2024 02:10

Merge branch 'master' into library-use-case

1d2961e

Use stable Rust for style/report and deactivate nightly x86_64 target…

b3c5a8d

… in CI

lucksus added 2 commits February 9, 2024 13:44

Reactivate nightly test job with continue-on-error set

7c632cf

continue-on-error if target=wasm32 or rust=nightly

ce34ca8

Skgland added 2 commits February 16, 2024 22:15

bump ahash lock to 0.8.7

90c8747

- fix nightly build - not bumping to latest aka. 0.8.8 as that has a msrv of 1.72.0 and we are only at 1.70.0

undo continue-on-error

7fce688

- currently `continue-on-error` is not shown in a usefull way on failiure see <https://github.com/orgs/community/discussions/15452>

Skgland added 4 commits February 16, 2024 23:18

fix compilation of wasm32 test and skip to run wasm32 tests

e5ad70c

cfg out benches for wasm32

7837c76

fix benchmarks being broken for every target except wam32

7028baa

fix build of run_iai bench for wasm32

e9982dc

mthom merged commit 84d5ce0 into mthom:master Feb 28, 2024
13 checks passed

triska mentioned this pull request Feb 28, 2024

Unexpected behaviour of lib_machine::Machine::run_query #2341

Closed

Skgland mentioned this pull request Aug 16, 2024

Value and QueryResolution serialization with serde #2493

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iron-out edge cases for library use-case, adding extensive real-world test assertions #2309

Iron-out edge cases for library use-case, adding extensive real-world test assertions #2309

lucksus commented Jan 29, 2024 •

edited

Loading

lucksus commented Jan 31, 2024

Skgland commented Jan 31, 2024 •

edited

Loading

mthom commented Feb 1, 2024 •

edited

Loading

lucksus commented Feb 1, 2024

lucksus commented Feb 1, 2024

Skgland commented Feb 1, 2024

lucksus commented Feb 1, 2024 •

edited

Loading

lucksus commented Feb 1, 2024

lucksus commented Feb 1, 2024

This comment was marked as resolved.

Skgland Feb 6, 2024

Skgland Feb 5, 2024

lucksus Feb 9, 2024

lucksus Feb 9, 2024

Skgland Feb 9, 2024

lucksus Feb 9, 2024 •

edited

Loading

Skgland Feb 16, 2024

lucksus commented Feb 9, 2024

Skgland commented Feb 9, 2024 •

edited

Loading

Skgland commented Feb 9, 2024

lucksus commented Feb 9, 2024

Skgland commented Feb 9, 2024

lucksus commented Feb 9, 2024

Skgland commented Feb 9, 2024

Skgland commented Feb 16, 2024

Skgland commented Feb 16, 2024

triska commented Feb 16, 2024

Skgland commented Feb 16, 2024

lucksus commented Feb 26, 2024

mthom commented Feb 28, 2024

aarroyoc commented Feb 28, 2024

@@ @@ -524,4 +531,82 @@ mod tests { @@
                           ),]))
                       );
                   }
+                  #[test]

Iron-out edge cases for library use-case, adding extensive real-world test assertions #2309

Iron-out edge cases for library use-case, adding extensive real-world test assertions #2309

Conversation

lucksus commented Jan 29, 2024 • edited Loading

Additions

Context

1. problems with discontiguous

2. Assertion 50 leaves run_query loop

lucksus commented Jan 31, 2024

Skgland commented Jan 31, 2024 • edited Loading

mthom commented Feb 1, 2024 • edited Loading

lucksus commented Feb 1, 2024

lucksus commented Feb 1, 2024

Skgland commented Feb 1, 2024

lucksus commented Feb 1, 2024 • edited Loading

lucksus commented Feb 1, 2024

lucksus commented Feb 1, 2024

This comment was marked as resolved.

Skgland Feb 6, 2024

Choose a reason for hiding this comment

Skgland Feb 5, 2024

Choose a reason for hiding this comment

lucksus Feb 9, 2024

Choose a reason for hiding this comment

lucksus Feb 9, 2024

Choose a reason for hiding this comment

Skgland Feb 9, 2024

Choose a reason for hiding this comment

lucksus Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Skgland Feb 16, 2024

Choose a reason for hiding this comment

lucksus commented Feb 9, 2024

Skgland commented Feb 9, 2024 • edited Loading

Skgland commented Feb 9, 2024

lucksus commented Feb 9, 2024

Skgland commented Feb 9, 2024

lucksus commented Feb 9, 2024

Skgland commented Feb 9, 2024

Skgland commented Feb 16, 2024

Skgland commented Feb 16, 2024

triska commented Feb 16, 2024

Skgland commented Feb 16, 2024

lucksus commented Feb 26, 2024

mthom commented Feb 28, 2024

aarroyoc commented Feb 28, 2024

lucksus commented Jan 29, 2024 •

edited

Loading

1. problems with `discontiguous`

2. Assertion 50 leaves `run_query` loop

Skgland commented Jan 31, 2024 •

edited

Loading

mthom commented Feb 1, 2024 •

edited

Loading

lucksus commented Feb 1, 2024 •

edited

Loading

lucksus Feb 9, 2024 •

edited

Loading

Skgland commented Feb 9, 2024 •

edited

Loading