[PATCH] D34584: Introduce new JSON import format
Kristof Beyls via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 29 00:32:24 PDT 2017
kristof.beyls accepted this revision.
kristof.beyls added a comment.
This revision is now accepted and ready to land.
In https://reviews.llvm.org/D34584#794519, @MatzeB wrote:
> Before I dive in the meta discussion, are there any issues with the patch itself?
I'm not up to speed on many parts of LNT that this code touches, but reading through it, I only added some optional nitpicks, apart from 1 issue that I think needs to be addressed before committing:
The patch as is will drop documentation on what field names to use in the json representation, making it impossible for people to write a converter from data from other testing/benchmarking systems to the json-format to import into LNT, without going through a lot of effort trying to reverse engineer the format from source code. I think the documentation of the fields with meaning for "nts" must be retained.
My experience is that most of such users of LNT don't grasp the concept of multiple "test-suite formats" such as "nts" or "compile" in the LNT DB, which is why I didn't bother trying to explain that well in the current documentation.
As you suggest later, maybe the json format should become self-describing (I'm maybe pushing your suggestion further than you intended here), but until that happens, I think the documentation on the nts field names that matter should stay.
Once that documentation issue is addressed, I have no objections for this patch to be committed.
>
>
> In https://reviews.llvm.org/D34584#793766, @kristof.beyls wrote:
>
>> In https://reviews.llvm.org/D34584#792859, @MatzeB wrote:
>>
>> > >> Drop Run/Info/tag: json itself is a schemaless format so it feels more natural without one. The LNT testsuite type is now determined by the submit URL used, or the '-s' commandline flag on the commandline. Both default to 'nts' if omitted.
>> > >
>> > > I don't fully understand how LNT reacts to this, but on the surface makes sense to me. If someone ever had time to do a major redesign of the database schema, there is a chance we might also be able to drop the need for differences in the database schema per test suite, like "nts"? Anyway, that is definitely far out of scope for this patch.
>> >
>> > The patch works fine. People using non-nts testsuites (which I have a strong suspicion only exist within apple), may need to supply a `-s MySchema` flag for the commandline tools or use `db_default/v4/MySchema/submitRun` instead of `db_default/submitRun` to express they don't use the 'nts' default.
>> >
>> > This is probably not the right patch to discuss LNT database design. But some points anyway:
>> >
>> > - IMO making SQL tables too generic (like having one row with MetricType, Test, Run and Value; so we could create new MetricTypes on demand) is a sure way to kill performance. As long as we use SQL we should have 1 table column for each metric.
>> > - A schema is nice when you need to decide bits like the unit of a metric, whether bigger is better or getting a human readable metric name for the website. However we could make good arguments that we only need that on the presentation/analysis side and not as part of the data storage model.
>>
>>
>> Agreed that not losing performance will be the key challenge for any redesign of the SQL schema to avoid having test-suite-specific tables. Anyway, it's indeed a discussion for another time...
>
>
> Again unrelated to this patch,
>
> Note that LNT is already designed to be extensible. Schemas like 'nts' or 'compile' switch the metrics available or what fields are considered for identifying/ordering runs. It just fails being user friendly so that people would actually create new schemes or modifying existing when they setup new benchmarks. IMO the current database design is good enough and we rather need a patch that makes creating and installing new schemes easy. I imagine a world where testsuite come with lnt.schema.json which describes the names/bigger is better/etc. for a testsuite and then when you setup a server you just reference those schemas in your lnt.cfg and get a server that matches your testsuite.
Right, seems like a reasonable way forward to me. I'm wondering if adding extra fields to a test-suite becoming as simple as just describing the extra fields in the json file you import, rather than at server configuration time, would even be nicer? Anyway - a discussion for the future.
>
>
>>
>>
>>>>> Previously:
>>>>>
>>>>> "Machine": {
>>>>>
>>>>> "name": "mymachine",
>>>>> "Info": {
>>>>> "hardware": "HAL 9000"
>>>>> }
>>>>>
>>>>> },
>>>>> "Run": {
>>>>>
>>>>> "Start Time": "2017-05-16 21:31:42",
>>>>> "End Time": "2017-05-16 21:31:42",
>>>>> "Info": {
>>>>> "__report_version__": "1",
>>>>
>>>> I don't know what __report_version__ is used for today, but would keep on having some kind of "report_version" key still help if in the future we'd want to change this format again? Having a version string in there could make it easier to support multiple versions?
>>>
>>> I talked a bit with Chris about this. And I think we want to persue a somwhat weaker version scheme were tools add something like "generated": "LNT version 0.4.1" or "generated": "llvm test-suite runner 0.1", similar in spirit to the http user agent header.
>>
>> I'm not sure I fully get this. I know of at least 2 groups having custom scripts to translate data coming out of their custom benchmarking system into something that can be imported into LNT. I know of yet another group planning to do something similar. I think it won't be possible for LNT to understand all possible producers of json files by name. So, with my limited insight here, I think that just a numerical version number or something similar indicating which version of the specification this json follows would be better than a string naming the producer?
>> In other words, I think this json format is rapidly becoming one of the key public interfaces of LNT - maybe the single most important one. If LNT keeps on growing in popularity in teams outside of LLVM, I think we'll end up having to specify this interface, and version control of it, a bit more strictly.
>
> Ok, I'll add a field '__format_version__' to the toplevel.
Thanks, sounds good.
>
>
>>
>>
>>>>> "tag": "nts",
>>>>> "run_order": "123456"
>>>>> }
>>>>>
>>>>> }
>>>>> Now:
>>>>>
>>>>> "machine": {
>>>>>
>>>>> "name": "mymachine",
>>>>> "hardware": "HAL 9000"
>>>>>
>>>>> },
>>>>> "run": {
>>>>>
>>>>> "start_time": "2017-05-16 21:31:42",
>>>>> "end_time": "2017-05-16 21:31:42",
>>>>> "llvm_project_revision": "123456"
>>>>
>>>> Since I see LNT being used also for tracking performance for non-LLVM projects, I think it'd be best to drop "llvm_" from "llvm_project_revision".
>>>
>>> - The name for the field is determined by the schema in LNT, so not necessarily part of this patch.
>>
>> OK, I agree it doesn't have to be part of this patch. But taking into account my remark above about the json file rapidly becoming one of the most important interfaces of LNT, I think that it may not be tenable in the long run to have the json fields map exactly to names of fields in the database schema. It is of course a nice to have; but the field names in the json file are on the publicly-visible interface of LNT, and the database field names are an implementation detail from a user point-of-view. I can foresee reasons why both might start deviating.
>> Since my gut feel is that it will be harder/more work to change the "json format specification" in the future, I thought that maybe it could be part of the change that is going to happen as part of this patch. Anyway, I don't have too strong a feeling about the exact name of this field.
>
> Yes and no; As see above about my vision of actually using the LNT feature of different benchmark suites instead of pushing everything into NTS :)
Right. As you said earlier, probably the only teams using different benchmark suite schemas may well be in Apple, so me and others using LNT elsewhere probably don't have the best feel for this.
I'm not against keeping the DB scheme as is. As you rightfully indicated, the key is making it easier to add fields for other test-suites.
>>> - "project_revision" seems to generic to me (I wouldn't know whether it's the compiler or the benchmark suites revision). I could probably live with "compiler_revision", but we should have a separate patch for that.
>>
>> Agreed, it's not the best possible name. It should be the revision of the software for which you're tracking the performance in my mind. That software doesn't have to be a "compiler". Not sure what a better name would be though.
>
> I'd be fine changing it to "compiler_revision" right away, if we need more discussion about how to call the field, then I'd suggest doing that in another patch to not delay this patch.
Let's change it to "compiler_revision" then, as all the other projects I know of so far track performance of a piece of software that could be claimed to be a compiler of sorts.
================
Comment at: docs/importing_data.rst:109-116
- * ".exec": represents execution time - a lower number is better.
- * ".exec.status": a non zero value represents a program failure.
- * ".score": represent a benchmark score - a higher number is better.
- * ".code_size": represents the code size of a program.
- * ".hash": represents a hash of the binary program being executed. This is
- used to detect if between compiler versions, the generated code has
- changed.
----------------
kristof.beyls wrote:
> I think the documentation of the different kinds of data that is interpreted by LNT should remain in the documentation in one form or another. It is essential information to transform test data into this format.
This is the one comment I feel needs to be addressed before committing this patch.
================
Comment at: lnt/server/db/testsuitedb.py:683
+ # Ignore incoming ids; we will create our own.
+ # TODO: Add some API/result to we can send a warning back to the user
+ # that we ignore the id.
----------------
s/to/so ?
================
Comment at: lnt/server/db/testsuitedb.py:888
# Construct the machine entry.
- machine,inserted = self._getOrCreateMachine(data['Machine'])
+ machine,inserted = self._getOrCreateMachine(data['machine'])
----------------
This is an opportunity to put a space after the comma in "machine,inserted" to make this PEP8 clean?
================
Comment at: lnt/server/db/testsuitedb.py:891
# Construct the run entry.
- run,inserted = self._getOrCreateRun(data['Run'], machine)
+ run,inserted = self._getOrCreateRun(data['run'], machine)
----------------
Another opportunity to make this PEP8-clean?
================
Comment at: lnt/testing/__init__.py:40
self.tests = list(tests)
- self.report_version = current_version
+ self.report_version = '1'
self.check()
----------------
I'm not sure, as I don't understand this code well, but there's a chance this should now be renamed to self.format_version instead of self.report_version?
================
Comment at: lnt/testing/__init__.py:109-112
if '__report_version__' in self.info:
raise ValueError("'__report_version__' key is reserved")
- self.info['__report_version__'] = str(current_version)
+ # TODO: Convert to version 2
+ self.info['__report_version__'] = '1'
----------------
I guess the conversion of the Run class to follow format_version 2, rather than 1 will fit more naturally with one of the follow-up patches? Or as a stand-alone technical-debt-removal patch?
Repository:
rL LLVM
https://reviews.llvm.org/D34584
More information about the llvm-commits
mailing list