[Bf-committers] Mantaflow landing and unit tests.

Fri Jan 3 09:34:01 CET 2020

Hi,

> how can we do better

Take responsibility for changes you do and get rid of "it's other's module
problem" would be a good start.
Not ignoring communication in this mailing list which doesn't include name
of your project would be a good continuation.
Making sure that a new project is added to the Release Notes should even
come without reminding.

The issue is not in Cycles and anyone familiar with physics/baking should
be able to make required changes to the tests suite (bake and update
reference image in this case).
If you have tried to follow suggested solution (which is, re-bake) you
would see how many bugs can be found in doing such a simple operation, and
why it's taking so long for Cycles team to make needed adjustments.

> We can take human element out of it, running the tests on a proposed patch

While i agree we should cover patches with CI to test for regressions and
code style, in this specific case it will be very tricky to do: regressions
tests themselves are to be updated (new solver is almost impossible to
configure to give pixel-perfect match with the old one). Since the
regression tests are in separate repository you would need to make a branch
first.

But then, if you've got a branch in SVN, just make a branch in Git as well
and use buildbot to run tests on all platforms. In fact, manta branch was
tested on buildbot prior to merge, and, sure enough, the tests didn't pass.

Now, if someone is not running tests locally and ignores tests failure on
the branch right before the merge I don't see how CI would have helped here.

> however nobody seems to be looking at the results otherwise we would have
figured out the tests were failing 3 weeks ago

Failure in this tests specific case I've pointed out right after the merge.
Regressions tests still failing in buildbot was pointed out on Monday 23rd
[1].

> and either disabled the test or fixed the problem.

Please don't propose disabling legit regression tests. Those are to be
fixed, not swiped under the carpet.

> I kinda feel that looking at the tests on a daily basis should be on
someones daily morning checklist, perhaps Nathan and/or Dalai can find
someone for this task?

I kind of expect it to be everyone's responsibility to:

- Verify tests are passing locally prior to commit
- Verify tests are passing on all platforms on buildbot.

The latter one you can very easily do by glancing at a single page [2]. If
the build is red something is wrong there, and is to be addressed. It could
either be a compilation error or regression test failure. Both of them are
to be solved and not ignored.

One cruel thing i can think about is to force everyone to pay more
attention is to prevent new build from being uploaded if regression tests
are failing. We'll hear very quickly from users that there are no new
builds on buildbot.

> What is remained to do?

There is a single failure now, which is related on liquid motion blur test.
I've got zero clue how this is supposed to be rendered now. I can bake it
and see fluid particles in viewport, but it's unclear how to make them
rendered.
Release Notes didn't give me any hints how to make liquid to render.

[1]
https://lists.blender.org/pipermail/bf-committers/2019-December/050381.html
[2] https://builder.blender.org/admin/#/builders

On Fri, Jan 3, 2020 at 3:50 AM Ray Molenkamp <ray at lazydodo.com> wrote:

> There's a couple of things we can look at:
>
> We can take human element out of it, running the tests on a proposed patch
> can and probably should be done automatically. I heard there were plans for
> this, I hope they materialize sooner rather than later. I think this is
> one of Nathans projects
>
> We are running the tests now on a nightly basis which is great, however
> nobody seems to be looking at the results otherwise we would have figured
> out the tests were failing 3 weeks ago and either disabled the test or
> fixed
> the problem. I kinda feel that looking at the tests on a daily basis should
> be on someones daily morning checklist, perhaps Nathan and/or Dalai can
> find
> someone for this task?
>
> as for commit messages, we have good guidance there [1], having people try
> a
> little harder will probably go a long way there.
>
> --Ray
>
> [1] https://wiki.blender.org/wiki/Style_Guide/Commit_Messages
>
>
>
> On 2020-01-02 7:04 p.m., Sebastián Barschkis wrote:
> > Hi Ray,
> >
> > you’re right, the Cycles tests need to be updated. They need to make use
> of the new fluid modifier.
> > Sergey pointed this out to me right after (within 12h) I landed the
> commits, i.e. the issue is known.
> >
> > So yes, blame it on me, I overlooked this part. I will look into this as
> soon as possible.
> >
> > I agree that this is not ideal. But apart from better commit messages,
> how can we do better at this time?
> >
> > Happy new year and best wishes,
> > Sebastián
> >
> >> On 2. Jan 2020, at 22:31, Ray Molenkamp <ray at lazydodo.com> wrote:
> >>
> >> All,
> >>
> >> Hate to be a heckler for running the unit tests, but please:
> >>
> >> When you land and/or review something big, RUN THE UNIT TESTS!
> >>
> >> When the monster 10 patch mantaflow patch landed, it broke
> >> 6 cycles unit tests on all platforms, 5 with different renders
> >> than the reference images and one full on blender crash [1]
> >>
> >> bda4a284d20164fec2433f7c40f49fc903319400 [2] fixed the render
> >> differences and turned the crash into a render difference [3]
> >>
> >> Unrelated side note: whats with the less than helpful
> >> commit message on this commit? it may as well have been
> >> committed with the message 'something fluid' or 'Tuesday'
> >> would have just as enlightening for other developers.
> >>
> >> To summarize:
> >>
> >> - The person submitting the patches has not run the tests
> >> - The reviewers have not run the tests
> >> - Less than optimal commit messages
> >> - 18!! days after landing, there is still a failing test
> >>
> >> Holiday season or not: I think we can and should do better than this
> >>
> >> --Ray
> >>
> >> [1] https://i.imgur.com/LE3baOg.png
> >> [2]
> https://developer.blender.org/rBbda4a284d20164fec2433f7c40f49fc903319400
> >> [3] https://i.imgur.com/5we0hEv.png
> >>
> >>
> >> _______________________________________________
> >> Bf-committers mailing list
> >> Bf-committers at blender.org
> >> https://lists.blender.org/mailman/listinfo/bf-committers
> > _______________________________________________
> > Bf-committers mailing list
> > Bf-committers at blender.org
> > https://lists.blender.org/mailman/listinfo/bf-committers
> _______________________________________________
> Bf-committers mailing list
> Bf-committers at blender.org
> https://lists.blender.org/mailman/listinfo/bf-committers
>

-- 
With best regards, Sergey Sharybin