1257954 - import_perf_data should account for new parent signature property

Status: RESOLVED FIXED

(Tree Management :: Perfherder, defect)

Description

[William Lachance (:wlach)]

Now that bug 1226693 has landed, we now have a "parent_signature" field for each performance signature which (optionally) references the parent of each subtest signature (if a test has no parent, this field is null/None). However, import_perf_data doesn't know about this, so tries to insert the subtest information into extra_properties (where we used to store this information).

Fixing this bug involves changing `treeherder/perf/management/commands/import_perf_data.py in the following ways:

1. First, only import those series with subtest_signatures into their dictionary. But remove subtest_signatures from the properties before inserting.
2. Then, iterate through the subtest signatures defined for those series one-by-one. Add those series, but set the `parent_signature` property to that of the parent.

We have a migration script for existing data which might help you understand what needs to be done here a bit:

https://github.com/mozilla/treeherder/blob/b08f904/treeherder/perf/management/commands/migrate_perf_signature_subtest_schema.py

Comment 1

[Anjana Vakil (:vakila)]

I'm interested in working on this, but I just want to make sure I've understood correctly:

1a. We should NOT import any data if the subtest_signatures property is missing from signature_props. Should we be making that check in _add_series (and e.g. return if subtest_signatures is not present) or in handle before we even append an _add_series call to futures, i.e. around https://github.com/mozilla/treeherder/blob/master/treeherder/perf/management/commands/import_perf_data.py#L155?

1b. We do not want to include subtest_signatures in the extra_properties dictionary when we create the PerformanceSignature object (around https://github.com/mozilla/treeherder/blob/master/treeherder/perf/management/commands/import_perf_data.py#L43). 

2. For each signature in subtest_signatures, we want to add the corresponding series just as it is currently done in _add_series, with the only change being that we want to set a parent_signature property in "defaults" when creating the PerformanceSignature object. 

Do I have it right so far? Am I missing anything?

Comment 2

[William Lachance (:wlach)]

(In reply to Anjana Vakil (:vakila) from comment #1)
> I'm interested in working on this, but I just want to make sure I've
> understood correctly:
> 
> 1a. We should NOT import any data if the subtest_signatures property is
> missing from signature_props. Should we be making that check in _add_series
> (and e.g. return if subtest_signatures is not present) or in handle before
> we even append an _add_series call to futures, i.e. around
> https://github.com/mozilla/treeherder/blob/master/treeherder/perf/management/
> commands/import_perf_data.py#L155?
> 
> 1b. We do not want to include subtest_signatures in the extra_properties
> dictionary when we create the PerformanceSignature object (around
> https://github.com/mozilla/treeherder/blob/master/treeherder/perf/management/
> commands/import_perf_data.py#L43). 

(1b): we want to import all the signatures, we just don't want to include the subtest_signatures in the extra properties (since that's not expressed in the parent_signature property).

> 2. For each signature in subtest_signatures, we want to add the
> corresponding series just as it is currently done in _add_series, with the
> only change being that we want to set a parent_signature property in
> "defaults" when creating the PerformanceSignature object.

Yes, this is correct.

Comment 3

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) from comment #2)
> (In reply to Anjana Vakil (:vakila) from comment #1)
> > I'm interested in working on this, but I just want to make sure I've
> > understood correctly:
> > 
> > 1a. We should NOT import any data if the subtest_signatures property is
> > missing from signature_props. Should we be making that check in _add_series
> > (and e.g. return if subtest_signatures is not present) or in handle before
> > we even append an _add_series call to futures, i.e. around
> > https://github.com/mozilla/treeherder/blob/master/treeherder/perf/management/
> > commands/import_perf_data.py#L155?
> > 
> > 1b. We do not want to include subtest_signatures in the extra_properties
> > dictionary when we create the PerformanceSignature object (around
> > https://github.com/mozilla/treeherder/blob/master/treeherder/perf/management/
> > commands/import_perf_data.py#L43). 
> 
> (1b): we want to import all the signatures, we just don't want to include
> the subtest_signatures in the extra properties (since that's not expressed
> in the parent_signature property).

"that's not" -> "that's now" :)

Comment 4

[GitHub Autolander Bot]

Attachment: [treeherder] vakila:import-perf-parent-sig > mozilla:master

Comment 5

[William Lachance (:wlach)]

Comment on attachment 8735933 [details] [review]
[treeherder] vakila:import-perf-parent-sig > mozilla:master

This is the right idea. Note that when bug 1260110 lands (hopefully tomorrow) we will no longer return a subtests property with the API, which will require you to change your approach slightly. Instead, we will return parent_signature as part of each signature (if it is present). So you will first insert all signatures *without* this property, then insert all of them with it.

Comment 6

[Anjana Vakil (:vakila)]

(In reply to William Lachance (:wlach) from comment #5)

Great, thank you for the comments (here and on Github). I'll wait on Bug 1260110 and then rewrite the patch.

Comment 7

[William Lachance (:wlach)]

(In reply to Anjana Vakil (:vakila) from comment #6)
> (In reply to William Lachance (:wlach) from comment #5)
> 
> Great, thank you for the comments (here and on Github). I'll wait on Bug
> 1260110 and then rewrite the patch.

Bug 1260110 has landed. :)

Comment 8

[William Lachance (:wlach)]

Comment on attachment 8735933 [details] [review]
[treeherder] vakila:import-perf-parent-sig > mozilla:master

Clearing review. Just give me confirmation that it's working for you locally and I'll give it a final once-over. If it still isn't working for you locally, feel free to ask more questions. Thanks for your patience!

Comment 9

[William Lachance (:wlach)]

Hi Anjana, are you still interested in working on this? I don't mind finishing it off if you've gotten busy or whatever, just let me know.

Comment 10

[Anjana Vakil (:vakila)]

Per the discussion on Github/IRC: Yes, I'm still working on it! Hopefully will be able to finish it up tonight or tomorrow, pending the issue I mentioned on Github.

Comment 11

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/3bc0c4283ef148ee8f366e85133be4174a2ec7b0
Bug 1257954 - Use parent signature property in import_perf_data

Modify the import_perf_data script to account for the new
parent_signature and has_subtests properties.
Add parent signatures before signatures with parents.
Use a new parent_hash argument in the _add_series function
to set the parent_signature property on the
PerformanceSignature object if necessary.

https://github.com/mozilla/treeherder/commit/3e5fb7f2d95339ad685a99a2ae56fdd1c5d560f2
Merge pull request #1375 from vakila/import-perf-parent-sig

Bug 1257954 - Use parent signature property in import_perf_data

Comment 12

[William Lachance (:wlach)]

Comment on attachment 8735933 [details] [review]
[treeherder] vakila:import-perf-parent-sig > mozilla:master

Great stuff!