[PATCH] D34132: Add scripts to perform LNT submissions

Kristof Beyls via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jun 20 07:03:24 PDT 2017


kristof.beyls added a comment.

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

In https://reviews.llvm.org/D34132#784401, @MatzeB wrote:

> In https://reviews.llvm.org/D34132#784019, @kristof.beyls wrote:
>
> > 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...

> 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.

Understood, makes sense.

> 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 :)

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?


Repository:
  rL LLVM

https://reviews.llvm.org/D34132





More information about the llvm-commits mailing list