-
Notifications
You must be signed in to change notification settings - Fork 315
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
test: add test for resumable sealing #1309
Conversation
7c78f84
to
b7f7b38
Compare
Ready for review. It's easiest to be reviewed as individual commits, as most part of it is a pure refactor without functional changes. |
de7cb91
to
0ebb788
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this works, but a less hyper-specific test (assuming exactly two total layers, using if to select from the only two options) would be a bit more versatile when considered against the range of behaviors the resumable seal feature might have. That said, we can update the tests as needed if behavior ever changes. I do think this does exercise what was implemented, though.
What I'm a little more concerned about is that this does not actually verify that resumable sealing does less work than the initial sealing, just that it doesn't fail or give wrong results. In that way, I'm not sure this test distinguishes between the observable behavior before and after implementation of the resumable sealing feature. Ideally, that's what this test would verify, though.
I think there may be some internal return values that track which layers were actually generated. Maybe those can be exploited, even if it means a slight change to the internal functions to make them more effectively testable.
@porcuquine it sounds like you're after a unit-test like thing along the lines of https://github.com/filecoin-project/rust-fil-proofs/blob/master/storage-proofs/porep/src/stacked/vanilla/proof.rs#L1567-L1692 |
@vmx I don't think it needs to be so thorough — just somehow capturing the central point of the feature (that resumable sealing is actually 'resuming') seems important. I'm not sure what mechanism would allow that minimally, but I think it can be 'minimal' if you see a way to make it be. |
To me the most important thing for this test is that resuming doesn't break anything. You can look at the logs if it does actually resume. I understand the point of having this automated, but I don't think it's worth the time/making the code more complicated. In case the resume doesn't work (it pretends to resume, but actually does a full rebuild) it would be bad performance wise, but things would still work. If we break this accidentally I expect that someone would report. Please also note that these tests only work on 2 layers as this is what the smaller sized parameters are doing. I would rather have performance tests on the actual parameters that run on a daily basis to catch regressions on the overall performance. One of those tests could also be one that resumes the sealing. To conclude: I would just merge this PR and don't bother too much about the resumable sealing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, @vmx, you convinced me. Approved.
Whoops, @vmx, looks like this need a rebase now. Ping me when done so we can merge. |
This should make a resumable sealing test easier to do.
0ebb788
to
9f0e2f3
Compare
@porcuquine rebased as tests pass. |
There is a short running version, which doesn't run the proofs and
a longer running one, which does run the proofs.
Currently tests fail when the proofs are running. I don't know why,
perhaps I'm removing files that are needed/not keeping files around
that should be kept.
It can easily be reproduced locally via:
@cryptonemo Can you please have a look at the test itself, if it does what
you expect and then please also check why it is failing?