Proptest index #28

Eh2406 · 2020-10-09T20:49:13Z

This is mostly porting proptests from Cargo. It is mostly ready for a first PR.

Things that have been intentionally left for a follow up:

The good tooling around reporting the part of the dependency_provider that was used.
The validation that a Ok(_) output has all its dependencies satisfied.
The validation that a Err(_) output happens when an SAT solver also can't find a solution.

This is going to be unpleasant unless we:

~~Impl a time out. So inputs that leave us in an infinite loop (or effectively infinite) just come out as a panic.~~ CC discussion on zulip
~~Some way to make sure the slow job of shrinking does not happen on CI. (not hard but I need to do it.)~~

Just wanted to share for others to take a look.

mpizenberg

I haven't properly read everything in details yet but this looks amazing!

tests/proptest.rs

mpizenberg · 2020-10-11T20:54:06Z

tests/proptest.rs

+#[should_panic]
+fn callback_can_panic() {
+    let mut dependency_provider = OfflineDependencyProvider::<_, NumberVersion>::new();
+    dependency_provider.add_dependencies(0, 0, vec![(666, Range::any())]);


ahahah good one

tests/proptest.rs

mpizenberg · 2020-10-11T21:12:29Z

tests/proptest.rs

+    })]
+
+    #[test]
+    /// This test is mostly for profiling


Because it uses strings for package ids right?

Yes. It was one of the first tests I wrote after getting registry_strategy working. It may no longer be pulling its weight. I wanted to compare a profile with expensive clones vs one with cheap clones. The other use the faster version ids to not waste CI time, but I want something to test that we are not cloning way way to much. I am open to suggestions.

It's good to still have a strings example in my opinion, to be closer to real usage when benchmarking things that depend on hash maps. If it's not needed on CI though, is there a way to deactivate a bunch of tests specifically when on CI?

There is always if std::env::var("CI").is_ok() {return;}. If it is still worth running then I am ok running on CI. When CI is a bottleneck we can revisit.

mpizenberg · 2020-10-11T21:16:13Z

tests/proptest.rs

+        for (name, ver) in cases {
+            let one = resolve(&TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), name, ver);
+            for _ in 0..3 {
+                match (&one, &resolve(&TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), name, ver)) {


Should we check that the difference is not due to the timeout?

Maybe, In Cargo we do not have fine grained control over the timeout. So I have not yet thought about how to take advantage of the better controls here.

tests/proptest.rs

mpizenberg · 2020-10-11T21:22:58Z

tests/proptest.rs

+        for (name, ver) in cases {
+            if resolve(&dependency_provider, name, ver).is_ok() {
+                prop_assert!(
+                    resolve(&TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), name, ver).is_ok(),


Should removing dependencies also ensure shorter runs?

That is a really good thought. That is a thing I did not think to test when working on Cargo as it is not flexible like that. Do you think we should work on it in this PR or in a follow up?

Nah, jut a thought I had but it's not correct. A missing dependency could generate an incompatibility that later reduces potential packages. But we should check that the difference, if there is one, is not due to timeouts I think.

Eh2406 · 2020-10-12T13:55:18Z

tests/proptest.rs

+            if resolve(&TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), name, ver).is_ok() {
+                prop_assert!(
+                    resolve(&TimeoutDependencyProvider::new(removed_provider.clone(), 50_000), name, ver).is_ok(),


I generally don't want to resolve on a generated dependency_provider without some kind of timeout. It is frustrating when CI fails with "terminated do to no output in 10 min" with no info on what input is slow. (looks like that limitation may have been from my Travis days, Github Actions may not have that limitation, but a hang is also frustrating.

What behaviour do you think the test should have when one of these hangs?

I think it's probable that those two won't take the same amount of time and it's not what this test tries to ensure, so if at least one of the two resolve timeouts the test should pass in my opinion. Passing or not passing that test will be much more correlated to the shape of index than to its size if there is an issue somewhere. In fact it would make sense to have different registry strategies, of different sizes and shapes, but that can be explored later.

Concretely, we can let it fail for now. Gather the examples when it fails to rerun them locally and figure if those are true failures. When a few of these happen we can see if it's worth continuing to fail or not.

We will definitely have to work on all of these tests, to ensure they maintain a good signal to noise ratio. If they turn out not to pull their weight, we can alter or remove!
Just to clarify are you ok with merging as is?

mpizenberg

Amazing work, thanks @Eh2406 !

The only thing I haven't spent time to look at closely is the proptest strategy to generate the index. I'll look at it later when I have time since this is obviously giving us good examples such as the ones we used for benchmarks.

Good to merge in my opinion, just waiting for @aleksator feedback

mpizenberg · 2020-10-14T08:18:55Z

Oh I had one remark actually that I forgot. The name callback sounds like something that should be called only once, when things have finished. I have hook that come to mind but there are probably better options.

aleksator · 2020-10-14T10:34:39Z

I added some proofreading commits and renamed ReversedDependencyProvider to ReverseDependencyProvider.
To see why that makes more sense try to read TimedoutDependencyProvider instead of TimeoutDependencyProvider as an analogy.

I've noticed there are a few unresolved discussions started by @mpizenberg (like Should we check that the difference is not due to the timeout?), but PR is already approved. Are we leaving those for later?

Eh2406 · 2020-10-14T13:49:46Z

Are we leaving those for later?

I interpret "Good to merge in my opinion, just waiting for @aleksator feedback" as a yes.

I like @aleksator's renames. I see the point about the name callback, can we have something more descriptive? Like callback and hook give no indication of why or when there called. Maybe check_up, check_in, heartbeat?

Also do we want that callback to take an argument in case we have diagnostics we can share about the progress?

mpizenberg · 2020-10-14T14:07:18Z

We can improve the callback API later, for now let's just find a more descriptive name and merge this. What do you think of checkpoint or loop_checkpoint?

Eh2406 · 2020-10-14T14:09:40Z

That works!

aleksator · 2020-10-14T14:32:23Z

Maybe check_interrupt[ion]?.. Hm.

aleksator · 2020-10-14T14:40:36Z

loop_checkpoint says where we are going to use that now, but maybe we could add usages elsewhere, so I would rather not encode that in the name.

Some other suggestions:
cancellation, should_cancel, should_interrupt.

Maybe we should discuss this on Zulip 😄

Eh2406 · 2020-10-14T14:44:42Z

https://rust-lang.zulipchat.com/#narrow/stream/260232-t-cargo.2FPubGrub/topic/How.20to.20add.20a.20timeout.3F

Eh2406 · 2020-10-16T01:09:44Z

rebased and updated the name to should_cancel.

mpizenberg · 2020-10-16T08:26:57Z

Any remarks left @aleksator ?

mpizenberg · 2020-10-16T09:05:04Z

aleksator merged commit 055fbec

damn I wasn't fast enought XD. Thanks @aleksator and amazing work @Eh2406 !

aleksator · 2020-10-16T09:16:03Z

damn I wasn't fast enought XD. Thanks @aleksator and amazing work @Eh2406 !

That's good, because I managed to edit commit messages 😋

Any remarks left @aleksator ?

Maybe we should have separated a timeout feature from proptest port. I realized that when I was adding a conventional commit label: it contained both feat: and test:. But oh well.

amazing work @Eh2406 !

Agree! 👍

Eh2406 mentioned this pull request Oct 10, 2020

Pick packages with a heuristic #20

Closed

Eh2406 marked this pull request as ready for review October 11, 2020 03:05

mpizenberg reviewed Oct 11, 2020

View reviewed changes

Eh2406 mentioned this pull request Oct 11, 2020

Benchmark v2 #34

Closed

Eh2406 force-pushed the proptest-index branch 2 times, most recently from 52534be to 5e35146 Compare October 12, 2020 02:57

Eh2406 commented Oct 12, 2020

View reviewed changes

Eh2406 force-pushed the proptest-index branch from 5e35146 to fceb5ee Compare October 12, 2020 21:07

mpizenberg approved these changes Oct 13, 2020

View reviewed changes

aleksator approved these changes Oct 14, 2020

View reviewed changes

Eh2406 added 12 commits October 15, 2020 21:03

a mostly direct port from Cargo

46e901d

clean it up some

40b5cc1

return OfflineDependencyProvider directly

ec95516

a first prop test

0174b76

if cloning Strings are so expensive try using ints

a483769

test allgorithm is deterministic

38b6418

poet over prop_minimum_version_errors_the_same

cbe77a4

port prop_removing_a_dep_cant_break

ab01e88

port prop_limited_independence_of_irrelevant_alternatives

65b52d0

port meta_test_deep_trees_from_strategy

7a2550d

ci: dont shrink on ci

c7ca561

tests: add a timeout

bf37863

Eh2406 and others added 6 commits October 15, 2020 21:03

suggestions

bc6d8e9

style: change license header to SPDX

d9b3f56

docs: link to DependencyProvider.callback

49e6e73

style: proofread naming and comments

4cc5531

style: rename ReversedDependencyProvider

64bbaa5

rename to should_cancel

bff9acd

Eh2406 force-pushed the proptest-index branch from d0f72c1 to bff9acd Compare October 16, 2020 01:09

aleksator approved these changes Oct 16, 2020

View reviewed changes

aleksator merged commit 055fbec into dev Oct 16, 2020

Eh2406 deleted the proptest-index branch October 16, 2020 15:00

Eh2406 mentioned this pull request Oct 16, 2020

test: compare with a sat solver #38

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proptest index #28

Proptest index #28

Eh2406 commented Oct 9, 2020 •

edited

Loading

mpizenberg left a comment

mpizenberg Oct 11, 2020

mpizenberg Oct 11, 2020

Eh2406 Oct 12, 2020

mpizenberg Oct 12, 2020

Eh2406 Oct 12, 2020

mpizenberg Oct 11, 2020

Eh2406 Oct 12, 2020

mpizenberg Oct 11, 2020

Eh2406 Oct 12, 2020

mpizenberg Oct 12, 2020

Eh2406 Oct 12, 2020

mpizenberg Oct 12, 2020

Eh2406 Oct 13, 2020

mpizenberg left a comment

mpizenberg commented Oct 14, 2020

aleksator commented Oct 14, 2020 •

edited

Loading

Eh2406 commented Oct 14, 2020

mpizenberg commented Oct 14, 2020

Eh2406 commented Oct 14, 2020

aleksator commented Oct 14, 2020

aleksator commented Oct 14, 2020

Eh2406 commented Oct 14, 2020

Eh2406 commented Oct 16, 2020

mpizenberg commented Oct 16, 2020

mpizenberg commented Oct 16, 2020

aleksator commented Oct 16, 2020 •

edited

Loading

Proptest index #28

Proptest index #28

Conversation

Eh2406 commented Oct 9, 2020 • edited Loading

mpizenberg 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

mpizenberg left a comment

Choose a reason for hiding this comment

mpizenberg commented Oct 14, 2020

aleksator commented Oct 14, 2020 • edited Loading

Eh2406 commented Oct 14, 2020

mpizenberg commented Oct 14, 2020

Eh2406 commented Oct 14, 2020

aleksator commented Oct 14, 2020

aleksator commented Oct 14, 2020

Eh2406 commented Oct 14, 2020

Eh2406 commented Oct 16, 2020

mpizenberg commented Oct 16, 2020

mpizenberg commented Oct 16, 2020

aleksator commented Oct 16, 2020 • edited Loading

Eh2406 commented Oct 9, 2020 •

edited

Loading

aleksator commented Oct 14, 2020 •

edited

Loading

aleksator commented Oct 16, 2020 •

edited

Loading