This is an archive of the discontinued LLVM Phabricator instance.

Add scripts to perform LNT submissions
AbandonedPublic

Authored by MatzeB on Jun 12 2017, 7:03 PM.

Details

Summary

The scripts are an alternative to using lnt runtest test-suite to
submit benchmarks to an LNT server. Having simple scripts in the
test-suite allows submissions without having lnt installed and gives
added flexibility in cases where lnt runtest is too strict.

The output is compatible with the LNT 'nts' testsuite format. (Long term
we really should create a new lnt schema that better matches the output
instead of having translation tables mapping lit names to LNT nts
names).

A typical test-suite run may look like this:

$ mkdir build
$ cd build
$ cmake -GNinja path/to/test-suite
$ ninja
$ lit -j1 -o lit.json .
$ path/to/test-suite/utils/lntsubmit.py \
    --name x86_default_config \
    --run-info compilerinfo.json \
    --url http://localhost:8000/db_default/submitRun \
    lit.json

If this accepted the next step is to change lnt runtest test-suite to use the scripts coming with the test-suite to avoid code duplication.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB created this revision.Jun 12 2017, 7:03 PM
MatzeB updated this revision to Diff 102288.Jun 12 2017, 7:26 PM

Rename tools to not use the .py extension; They are used as standalone tools so there is no need to bake the .py extension into the name (no extensions is shorter and in principle allows replacing them with something written in a different language)

cmatthews edited edge metadata.Jun 13 2017, 1:50 PM

Tests please?

MatzeB updated this revision to Diff 102432.Jun 13 2017, 3:12 PM
  • Fix error handling in compilerinfo
  • Add tests
MatzeB updated this revision to Diff 102433.Jun 13 2017, 3:13 PM

and add missing notclang helper

MatzeB updated this revision to Diff 102438.Jun 13 2017, 3:18 PM

add notclang tool for real this time.

kristof.beyls edited edge metadata.Jun 19 2017, 7:13 AM

There is quite a bit of code that is duplicated from LNT to test-suite as part of this patch.
I'm not entirely sure if having this much code duplication between LNT and test-suite is desirable, unless this is part of a bigger redesign, like we discussed in January in the thread starting roughly at http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170123/423073.html, with a nicer design in the end?

As I said before, I like the conceptual idea of having the functionality to drive the test-suite being part of the "test-suite" project rather than the "LNT" project, but I haven't thought further about practical consequences, like potential code duplication, showing up here.
My gut feel is that most of these issues could go away in the end if we'd put all code to drive the test-suite in test-suite, and make LNT "just" the web server/database/analysis functionality.
But it's probably best to make sure we agree that's where we want to end up before adding a lot of code duplication as part of this first step patch?

Apart from my other thoughts above, I think this is the point where it is useful to start adding some documentation on how to run the tests for the test-suite framework, e.g. similar in spirit to http://lnt.readthedocs.io/en/latest/developer_guide.html.

utils/lntsubmit
29

s/jenkins/LNT/?

utils/tests/lntsubmit-minimal.test
2

using tail and head with hard-coded numbers seems a bit too icky to me.
Would something like:

RUN: grep -v '^#' %s > %s

be nicer? We'd of course have to add # signs at the beginning of lines that aren't the JSON-input. I think that gives the advantage that the test files are slightly easier to parse as a human, and they look more like other test files using FileCheck in various LLVM projects.

There is quite a bit of code that is duplicated from LNT to test-suite as part of this patch.
I'm not entirely sure if having this much code duplication between LNT and test-suite is desirable, unless this is part of a bigger redesign, like we discussed in January in the thread starting roughly at http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170123/423073.html, with a nicer design in the end?

  1. Yes this breaks the dependency on LNT for the test running and data submission as discussed. Doesn't feel like a "bigger redesign" to me though as this patch is mostly it.
  2. I will resolve the duplication between utils/lntsubmit and lnt/tests/test_suite.py in a followup by making the latter use the former.
  3. The duplication between utils/compilerinfo and lnt/testing/util/compilers.py is trickier as we need to keep the code in LNT for the runtest nts and runtest compile modes. At the same time the point here is to break dependencies to LNT so we shouldn't use LNTs code. That leaves us those possibilities:
    • The radical solution: We stop adding all the compilerinfo to each submission. I'm not gonna start that discussion here :)
    • Not copying the code but writing a new version of it. But what's the point...
    • I actually have a commit that refactors LNTs compilerinfo file to look exactly like the one here by moving lnt/testing/util/compilers.py to a new lnt/tools/compilerinfo.py and adding the json printing bits when it is run as a standalone tool while the other LNT code can keep importing it. At that point we could keep LNT and test-suite in sync by copying that file over. It just felt slightly unmotivated/out of place on the LNT side. But I can submit that patch again if you prefer the way it makes it easier to keep the two in sync.
    • We are only talking about 300 lines of code here. Yes code duplication is bad, but we have to weigh that against the costs of having LNT as a dependency for running the test-suite. (Or if we would choose to do so the cost of factoring the common functionality out into a shared library).

As I said before, I like the conceptual idea of having the functionality to drive the test-suite being part of the "test-suite" project rather than the "LNT" project, but I haven't thought further about practical consequences, like potential code duplication, showing up here.
My gut feel is that most of these issues could go away in the end if we'd put all code to drive the test-suite in test-suite, and make LNT "just" the web server/database/analysis functionality.
But it's probably best to make sure we agree that's where we want to end up before adding a lot of code duplication as part of this first step patch?

As a data point: We already have at least 1 internal benchmark that works by uploading the data directly to the submitRun URL on the jenkins server instead of going through lnt runtest. And as far as I know that works fine (though I'm not directly involved in setting this up/maintaining it).

Apart from my other thoughts above, I think this is the point where it is useful to start adding some documentation on how to run the tests for the test-suite framework, e.g. similar in spirit to http://lnt.readthedocs.io/en/latest/developer_guide.html.

Yes I can do that.

Thanks for sharing your thoughts - that makes things quite a bit clearer to me.

There is quite a bit of code that is duplicated from LNT to test-suite as part of this patch.
I'm not entirely sure if having this much code duplication between LNT and test-suite is desirable, unless this is part of a bigger redesign, like we discussed in January in the thread starting roughly at http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170123/423073.html, with a nicer design in the end?

  1. Yes this breaks the dependency on LNT for the test running and data submission as discussed. Doesn't feel like a "bigger redesign" to me though as this patch is mostly it.

I was thinking of having all functionality of "lnt runtest test-suite" moved to the test-suite being the end goal.
That probably also isn't really a "bigger redesign", although it would involve moving more code and having potentially a deeper think about the regression tests for "lnt runtest test-suite" living somewhere in the test-suite project.
It might turn out to all be pretty trivial - I haven't explored the idea further.
I'm afraid I don't have the bandwidth to explore this idea further in the near future myself...

  1. I will resolve the duplication between utils/lntsubmit and lnt/tests/test_suite.py in a followup by making the latter use the former.

Understood, makes sense.

  1. The duplication between utils/compilerinfo and lnt/testing/util/compilers.py is trickier as we need to keep the code in LNT for the runtest nts and runtest compile modes. At the same time the point here is to break dependencies to LNT so we shouldn't use LNTs code. That leaves us those possibilities:
    • The radical solution: We stop adding all the compilerinfo to each submission. I'm not gonna start that discussion here :)

I'll keep my mouth shut then on this aspect ;)

  • Not copying the code but writing a new version of it. But what's the point...

Agreed - the worst of the alternatives here.

  • I actually have a commit that refactors LNTs compilerinfo file to look exactly like the one here by moving lnt/testing/util/compilers.py to a new lnt/tools/compilerinfo.py and adding the json printing bits when it is run as a standalone tool while the other LNT code can keep importing it. At that point we could keep LNT and test-suite in sync by copying that file over. It just felt slightly unmotivated/out of place on the LNT side. But I can submit that patch again if you prefer the way it makes it easier to keep the two in sync.

Right, it seems it's a necessary evil to have 2 copies of this functionality, one in the test-suite, one in LNT. Even though this functionality probably doesn't change often, I think it'd be better if the implementation was identical between LNT and test-suite so it is easier to keep in sync in the future.

  • We are only talking about 300 lines of code here. Yes code duplication is bad, but we have to weigh that against the costs of having LNT as a dependency for running the test-suite. (Or if we would choose to do so the cost of factoring the common functionality out into a shared library).

If this is the only code to share, I think creating a separate project to be able share this code is worse than having this level of code duplication, especially if the 2 version can be kept in sync easily. Let's go with the compilerinfo functionality being duplicated.

I hope that if we'd end up moving all functionality that exists in "lnt runtest test-suite" to the test-suite project, we'd discover this is the only functionality that needs to be duplicated.

As I said before, I like the conceptual idea of having the functionality to drive the test-suite being part of the "test-suite" project rather than the "LNT" project, but I haven't thought further about practical consequences, like potential code duplication, showing up here.
My gut feel is that most of these issues could go away in the end if we'd put all code to drive the test-suite in test-suite, and make LNT "just" the web server/database/analysis functionality.
But it's probably best to make sure we agree that's where we want to end up before adding a lot of code duplication as part of this first step patch?

As a data point: We already have at least 1 internal benchmark that works by uploading the data directly to the submitRun URL on the jenkins server instead of going through lnt runtest. And as far as I know that works fine (though I'm not directly involved in setting this up/maintaining it).

Right. Let me add another data point: I know of at least 2 teams working on other projects than LLVM that use the LNT server/database/analysis functionality to track performance. They've created scripts to create the json format LNT expects from their benchmark suites. For them the "lnt runtest" functionality is meaningless. It's part of why I believe the "lnt runtest test-suite" functionality probably would better live in the test-suite project rather than the LNT project.

Apart from my other thoughts above, I think this is the point where it is useful to start adding some documentation on how to run the tests for the test-suite framework, e.g. similar in spirit to http://lnt.readthedocs.io/en/latest/developer_guide.html.

Yes I can do that.

In summary: I'm happy with this change, thanks for the further explanations!
@cmatthews: any remaining objections from your side?

cmatthews accepted this revision.Jun 23 2017, 12:53 PM

Looks great! Thanks Matthias!

This revision is now accepted and ready to land.Jun 23 2017, 12:53 PM
MatzeB abandoned this revision.Sep 10 2021, 10:05 AM