Re: [Mesa-dev] How to merge Mesa changes which require corresponding piglit changes
Michel Dänzer writes: > On 2019-12-02 3:15 p.m., Tapani Pälli wrote: >> On 11/15/19 8:41 PM, Mark Janes wrote: >>> Michel Dänzer writes: >>> On 2019-11-15 4:02 p.m., Mark Janes wrote: > Michel Dänzer writes: > >> Now that the GitLab CI pipeline tests a snapshot of piglit with >> llvmpipe >> (https://gitlab.freedesktop.org/mesa/mesa/merge_requests/2468), the >> question has come up how to deal with inter-dependent Mesa/piglit >> changes (where merging only one or the other causes some piglit >> regressions). >> >> >> First of all, let it be clear that just merging the Mesa changes as-is >> and breaking the GitLab CI pipeline is not acceptable. >> >> >> From the Mesa POV, the easiest solution is: >> >> 1. Merge the piglit changes >> 2. In the Mesa MR (merge request), add a commit which updates >> piglit[0] >> 3. If the CI pipeline is green, the MR can be merged >> >> >> In case one wants to avoid alarms from external CI systems, another >> possibility is: > > For the Intel CI, no alarm is generated if the piglit test is pushed > first. Normal development process includes writing a piglit test to > illustrate the bug that is being fixed. Cool, but what if the piglit changes affect the results of existing tests? That was the situation yesterday which prompted this thread. >>> >>> We attribute the status change to piglit in the CI config, within a few >>> hours. The test shows up as a failure in CI until it is triaged. >> >> I think we have a problem with current gitlab CI process. >> >> Right now if someone needs to update piglit commit used by CI, he also >> ends up fixing and editing the .gitlab-ci/piglit/quick_gl.txt (and >> glslparser+quick_shader.txt) as CI reports numerous failures because of >> completely unrelated stuff as meanwhile people added other tests, >> removed tests and modified them. > > This is at least somewhat intentional, as the results of any newly added > tests should be carefully checked for plausibility. If a piglit (or any other suite) commit causes a test failure, the failure is not a Mesa regression, by definition. CI is for identifying regressions. The simple fact that a failure is due to a non-Mesa commit means it can be immediately masked in CI. >> I think we should turn such warnings on only when we have more >> sophisticated algorithm to detect actual regression (not just 'state >> change', like additional test or removed test). > > It's unclear what exactly you're proposing. In order to catch > regressions (e.g. pass -> warn, pass -> fail, pass -> skip, pass -> > crash), we need a list of all tests on at least one side of each > transition. We're currently keeping the list of all > warning/failing/skipped/crashing tests, but not passing tests (to keep > the lists as small as possible). CI must track the development of the test suites to capture the the required transitions for tests. If CI does not track each test suite commit, then some developer (eg Tapani) has to go and triage test results from other piglit committers in order to deploy tests in CI. This is a barrier to test-first development, and it is also unfair to the developers that are diligent with testing. Piglit and Crucible are maintained by the Mesa community and it makes sense that Mesa CI should track their development. Tracking other test suites (dEQP, CTS, etc) means that the Mesa community may be distracted by test failures that are bugs in the suite instead of bugs in Mesa. Mesa developers are not enabled to fix bugs in dEQP. However, tracking external suites also identifies new conformance requirements that Mesa will eventually be required to pass. In practice, some test suites are easy to track and have developers that are eager to resolve issues that are identified by the Mesa community (eg dEQP, VulkanCTS). Other suites are in a constant state of build churn and are hard to track (Skia). Tracking test suites can be done without too much effort, but it requires a centralized role similar to a release manager. > One possibility might be to remove the summary at the end of the lists. > That would allow new passing tests to be silently added, but it would > mean we could no longer catch pass -> notrun regressions. > > > -- > Earthling Michel Dänzer | https://redhat.com > Libre software enthusiast | Mesa and X developer ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] How to merge Mesa changes which require corresponding piglit changes
On 12/2/19 5:25 PM, Michel Dänzer wrote: On 2019-12-02 3:15 p.m., Tapani Pälli wrote: On 11/15/19 8:41 PM, Mark Janes wrote: Michel Dänzer writes: On 2019-11-15 4:02 p.m., Mark Janes wrote: Michel Dänzer writes: Now that the GitLab CI pipeline tests a snapshot of piglit with llvmpipe (https://gitlab.freedesktop.org/mesa/mesa/merge_requests/2468), the question has come up how to deal with inter-dependent Mesa/piglit changes (where merging only one or the other causes some piglit regressions). First of all, let it be clear that just merging the Mesa changes as-is and breaking the GitLab CI pipeline is not acceptable. From the Mesa POV, the easiest solution is: 1. Merge the piglit changes 2. In the Mesa MR (merge request), add a commit which updates piglit[0] 3. If the CI pipeline is green, the MR can be merged In case one wants to avoid alarms from external CI systems, another possibility is: For the Intel CI, no alarm is generated if the piglit test is pushed first. Normal development process includes writing a piglit test to illustrate the bug that is being fixed. Cool, but what if the piglit changes affect the results of existing tests? That was the situation yesterday which prompted this thread. We attribute the status change to piglit in the CI config, within a few hours. The test shows up as a failure in CI until it is triaged. I think we have a problem with current gitlab CI process. Right now if someone needs to update piglit commit used by CI, he also ends up fixing and editing the .gitlab-ci/piglit/quick_gl.txt (and glslparser+quick_shader.txt) as CI reports numerous failures because of completely unrelated stuff as meanwhile people added other tests, removed tests and modified them. This is at least somewhat intentional, as the results of any newly added tests should be carefully checked for plausibility. I think we should turn such warnings on only when we have more sophisticated algorithm to detect actual regression (not just 'state change', like additional test or removed test). It's unclear what exactly you're proposing. In order to catch regressions (e.g. pass -> warn, pass -> fail, pass -> skip, pass -> crash), we need a list of all tests on at least one side of each transition. We're currently keeping the list of all warning/failing/skipped/crashing tests, but not passing tests (to keep the lists as small as possible). One possibility might be to remove the summary at the end of the lists. That would allow new passing tests to be silently added, but it would mean we could no longer catch pass -> notrun regressions. Yeah, the last point is what I had in mind but it is tricky .. I guess I don't really have a good concrete proposal currently but I was hoping maybe someone comes up with one :) I guess my issues boil down to difference vs Intel CI that there we track Piglit master so the overall state is 'more fresh'. With current gitlab CI the issues come late as many commits may have happened. So the person dealing with the issue (updating tag) does not have the context of those changes or maybe even expertise about the changes (and what was expected result), it should've have been caught already earlier. It could be also that I'm trying to update too big chunk at once, should go commit by commit and see what happens to the results. // Tapani ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] How to merge Mesa changes which require corresponding piglit changes
On 2019-12-02 3:15 p.m., Tapani Pälli wrote: > On 11/15/19 8:41 PM, Mark Janes wrote: >> Michel Dänzer writes: >> >>> On 2019-11-15 4:02 p.m., Mark Janes wrote: Michel Dänzer writes: > Now that the GitLab CI pipeline tests a snapshot of piglit with > llvmpipe > (https://gitlab.freedesktop.org/mesa/mesa/merge_requests/2468), the > question has come up how to deal with inter-dependent Mesa/piglit > changes (where merging only one or the other causes some piglit > regressions). > > > First of all, let it be clear that just merging the Mesa changes as-is > and breaking the GitLab CI pipeline is not acceptable. > > > From the Mesa POV, the easiest solution is: > > 1. Merge the piglit changes > 2. In the Mesa MR (merge request), add a commit which updates > piglit[0] > 3. If the CI pipeline is green, the MR can be merged > > > In case one wants to avoid alarms from external CI systems, another > possibility is: For the Intel CI, no alarm is generated if the piglit test is pushed first. Normal development process includes writing a piglit test to illustrate the bug that is being fixed. >>> >>> Cool, but what if the piglit changes affect the results of existing >>> tests? That was the situation yesterday which prompted this thread. >> >> We attribute the status change to piglit in the CI config, within a few >> hours. The test shows up as a failure in CI until it is triaged. > > I think we have a problem with current gitlab CI process. > > Right now if someone needs to update piglit commit used by CI, he also > ends up fixing and editing the .gitlab-ci/piglit/quick_gl.txt (and > glslparser+quick_shader.txt) as CI reports numerous failures because of > completely unrelated stuff as meanwhile people added other tests, > removed tests and modified them. This is at least somewhat intentional, as the results of any newly added tests should be carefully checked for plausibility. > I think we should turn such warnings on only when we have more > sophisticated algorithm to detect actual regression (not just 'state > change', like additional test or removed test). It's unclear what exactly you're proposing. In order to catch regressions (e.g. pass -> warn, pass -> fail, pass -> skip, pass -> crash), we need a list of all tests on at least one side of each transition. We're currently keeping the list of all warning/failing/skipped/crashing tests, but not passing tests (to keep the lists as small as possible). One possibility might be to remove the summary at the end of the lists. That would allow new passing tests to be silently added, but it would mean we could no longer catch pass -> notrun regressions. -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] How to merge Mesa changes which require corresponding piglit changes
Hi; On 11/15/19 8:41 PM, Mark Janes wrote: Michel Dänzer writes: On 2019-11-15 4:02 p.m., Mark Janes wrote: Michel Dänzer writes: Now that the GitLab CI pipeline tests a snapshot of piglit with llvmpipe (https://gitlab.freedesktop.org/mesa/mesa/merge_requests/2468), the question has come up how to deal with inter-dependent Mesa/piglit changes (where merging only one or the other causes some piglit regressions). First of all, let it be clear that just merging the Mesa changes as-is and breaking the GitLab CI pipeline is not acceptable. From the Mesa POV, the easiest solution is: 1. Merge the piglit changes 2. In the Mesa MR (merge request), add a commit which updates piglit[0] 3. If the CI pipeline is green, the MR can be merged In case one wants to avoid alarms from external CI systems, another possibility is: For the Intel CI, no alarm is generated if the piglit test is pushed first. Normal development process includes writing a piglit test to illustrate the bug that is being fixed. Cool, but what if the piglit changes affect the results of existing tests? That was the situation yesterday which prompted this thread. We attribute the status change to piglit in the CI config, within a few hours. The test shows up as a failure in CI until it is triaged. I think we have a problem with current gitlab CI process. Right now if someone needs to update piglit commit used by CI, he also ends up fixing and editing the .gitlab-ci/piglit/quick_gl.txt (and glslparser+quick_shader.txt) as CI reports numerous failures because of completely unrelated stuff as meanwhile people added other tests, removed tests and modified them. I think we should turn such warnings on only when we have more sophisticated algorithm to detect actual regression (not just 'state change', like additional test or removed test). 1. In the Mesa MR, add a commit which disables the piglit tests broken by the Mesa changes. 2. If the CI pipeline is green, the MR can be merged 3. Merge the piglit changes 4. Create another Mesa MR which updates piglit[0] and re-enables the tests disabled in step 1 I hope that covers it, don't hesitate to ask questions if something's still unclear. It might help developers if CI generated the patch to make their pipeline pass. It does for the test result list, if that's what you mean. However, that patch shouldn't be applied mechanically, but only after confirming that all changes in test results are expected. Ideally, whenever there are any new tests, the corresponding CI jobs should be run several times to make sure the new results are stable, otherwise any flaky tests should be excluded. -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev // Tapani ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] How to merge Mesa changes which require corresponding piglit changes
Michel Dänzer writes: > On 2019-11-15 4:02 p.m., Mark Janes wrote: >> Michel Dänzer writes: >> >>> Now that the GitLab CI pipeline tests a snapshot of piglit with llvmpipe >>> (https://gitlab.freedesktop.org/mesa/mesa/merge_requests/2468), the >>> question has come up how to deal with inter-dependent Mesa/piglit >>> changes (where merging only one or the other causes some piglit >>> regressions). >>> >>> >>> First of all, let it be clear that just merging the Mesa changes as-is >>> and breaking the GitLab CI pipeline is not acceptable. >>> >>> >>> From the Mesa POV, the easiest solution is: >>> >>> 1. Merge the piglit changes >>> 2. In the Mesa MR (merge request), add a commit which updates piglit[0] >>> 3. If the CI pipeline is green, the MR can be merged >>> >>> >>> In case one wants to avoid alarms from external CI systems, another >>> possibility is: >> >> For the Intel CI, no alarm is generated if the piglit test is pushed >> first. Normal development process includes writing a piglit test to >> illustrate the bug that is being fixed. > > Cool, but what if the piglit changes affect the results of existing > tests? That was the situation yesterday which prompted this thread. We attribute the status change to piglit in the CI config, within a few hours. The test shows up as a failure in CI until it is triaged. >>> 1. In the Mesa MR, add a commit which disables the piglit tests broken >>> by the Mesa changes. >>> 2. If the CI pipeline is green, the MR can be merged >>> 3. Merge the piglit changes >>> 4. Create another Mesa MR which updates piglit[0] and re-enables the >>> tests disabled in step 1 >>> >>> I hope that covers it, don't hesitate to ask questions if something's >>> still unclear. >> >> It might help developers if CI generated the patch to make their pipeline >> pass. > > It does for the test result list, if that's what you mean. > > However, that patch shouldn't be applied mechanically, but only after > confirming that all changes in test results are expected. Ideally, > whenever there are any new tests, the corresponding CI jobs should be > run several times to make sure the new results are stable, otherwise any > flaky tests should be excluded. > > > -- > Earthling Michel Dänzer | https://redhat.com > Libre software enthusiast | Mesa and X developer ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] How to merge Mesa changes which require corresponding piglit changes
On 2019-11-15 4:02 p.m., Mark Janes wrote: > Michel Dänzer writes: > >> Now that the GitLab CI pipeline tests a snapshot of piglit with llvmpipe >> (https://gitlab.freedesktop.org/mesa/mesa/merge_requests/2468), the >> question has come up how to deal with inter-dependent Mesa/piglit >> changes (where merging only one or the other causes some piglit >> regressions). >> >> >> First of all, let it be clear that just merging the Mesa changes as-is >> and breaking the GitLab CI pipeline is not acceptable. >> >> >> From the Mesa POV, the easiest solution is: >> >> 1. Merge the piglit changes >> 2. In the Mesa MR (merge request), add a commit which updates piglit[0] >> 3. If the CI pipeline is green, the MR can be merged >> >> >> In case one wants to avoid alarms from external CI systems, another >> possibility is: > > For the Intel CI, no alarm is generated if the piglit test is pushed > first. Normal development process includes writing a piglit test to > illustrate the bug that is being fixed. Cool, but what if the piglit changes affect the results of existing tests? That was the situation yesterday which prompted this thread. >> 1. In the Mesa MR, add a commit which disables the piglit tests broken >> by the Mesa changes. >> 2. If the CI pipeline is green, the MR can be merged >> 3. Merge the piglit changes >> 4. Create another Mesa MR which updates piglit[0] and re-enables the >> tests disabled in step 1 >> >> I hope that covers it, don't hesitate to ask questions if something's >> still unclear. > > It might help developers if CI generated the patch to make their pipeline > pass. It does for the test result list, if that's what you mean. However, that patch shouldn't be applied mechanically, but only after confirming that all changes in test results are expected. Ideally, whenever there are any new tests, the corresponding CI jobs should be run several times to make sure the new results are stable, otherwise any flaky tests should be excluded. -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] How to merge Mesa changes which require corresponding piglit changes
Michel Dänzer writes: > Now that the GitLab CI pipeline tests a snapshot of piglit with llvmpipe > (https://gitlab.freedesktop.org/mesa/mesa/merge_requests/2468), the > question has come up how to deal with inter-dependent Mesa/piglit > changes (where merging only one or the other causes some piglit > regressions). > > > First of all, let it be clear that just merging the Mesa changes as-is > and breaking the GitLab CI pipeline is not acceptable. > > > From the Mesa POV, the easiest solution is: > > 1. Merge the piglit changes > 2. In the Mesa MR (merge request), add a commit which updates piglit[0] > 3. If the CI pipeline is green, the MR can be merged > > > In case one wants to avoid alarms from external CI systems, another > possibility is: For the Intel CI, no alarm is generated if the piglit test is pushed first. Normal development process includes writing a piglit test to illustrate the bug that is being fixed. > 1. In the Mesa MR, add a commit which disables the piglit tests broken > by the Mesa changes. > 2. If the CI pipeline is green, the MR can be merged > 3. Merge the piglit changes > 4. Create another Mesa MR which updates piglit[0] and re-enables the > tests disabled in step 1 > > I hope that covers it, don't hesitate to ask questions if something's > still unclear. It might help developers if CI generated the patch to make their pipeline pass. > [0] How to update piglit in the CI pipeline: > > * Change the commit hash on the "git checkout" line in > .gitlab-ci/debian-test-install.sh[1] to a later commit from the piglit > master branch > * Bump DEBIAN_TEST_TAG[1] in .gitlab-ci.yml to the current date > * May also need to update .gitlab-ci/piglit/*.txt to match any expected > changes in test results > > See https://gitlab.freedesktop.org/mesa/mesa/merge_requests/2748 for an > example. > > > [1] Might soon be .gitlab-ci/container/x86_test.sh and DEBIAN_TAG in the > x86_test job definition, respectively, once > https://gitlab.freedesktop.org/mesa/mesa/merge_requests/2722 is merged. > > > -- > Earthling Michel Dänzer | https://redhat.com > Libre software enthusiast | Mesa and X developer > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] How to merge Mesa changes which require corresponding piglit changes
Now that the GitLab CI pipeline tests a snapshot of piglit with llvmpipe (https://gitlab.freedesktop.org/mesa/mesa/merge_requests/2468), the question has come up how to deal with inter-dependent Mesa/piglit changes (where merging only one or the other causes some piglit regressions). First of all, let it be clear that just merging the Mesa changes as-is and breaking the GitLab CI pipeline is not acceptable. From the Mesa POV, the easiest solution is: 1. Merge the piglit changes 2. In the Mesa MR (merge request), add a commit which updates piglit[0] 3. If the CI pipeline is green, the MR can be merged In case one wants to avoid alarms from external CI systems, another possibility is: 1. In the Mesa MR, add a commit which disables the piglit tests broken by the Mesa changes. 2. If the CI pipeline is green, the MR can be merged 3. Merge the piglit changes 4. Create another Mesa MR which updates piglit[0] and re-enables the tests disabled in step 1 I hope that covers it, don't hesitate to ask questions if something's still unclear. [0] How to update piglit in the CI pipeline: * Change the commit hash on the "git checkout" line in .gitlab-ci/debian-test-install.sh[1] to a later commit from the piglit master branch * Bump DEBIAN_TEST_TAG[1] in .gitlab-ci.yml to the current date * May also need to update .gitlab-ci/piglit/*.txt to match any expected changes in test results See https://gitlab.freedesktop.org/mesa/mesa/merge_requests/2748 for an example. [1] Might soon be .gitlab-ci/container/x86_test.sh and DEBIAN_TAG in the x86_test job definition, respectively, once https://gitlab.freedesktop.org/mesa/mesa/merge_requests/2722 is merged. -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
