Closed Bug 1351010 Opened 4 years ago Closed 4 years ago

Return a single value from optimize functions


(Firefox Build System :: Task Configuration, task)

Not set


(Not tracked)



(Reporter: dustin, Assigned: dustin, Mentored)



(Keywords: good-first-bug)


(4 files, 1 obsolete file)

Optimization functions look like this:

def opt_index_search(task, params, index_path):
        task_id = find_task_id(

        return True, task_id
    except requests.exceptions.HTTPError:

    return False, None

they return a boolean and a taskId.  However, there are really only three possibilities:

 - don't optimize (False, None)
 - optimize away (True, None)
 - optimize by replacing with existing task (True, taskId)

So we could replace those with a single value that is either False, True, or a taskId.  That would be a little simpler to pass around.
Hi Dustin,

I have attached a work in progress patch for this bug. I made my changes in the "opt_index_search" function of

I assume some of the other functions in need to be modified as well, please let me know which ones.

Lastly, could be you please assign this bug to me. I think its a great bug for me to get familiarized with the Bugzilla workflow.

Flags: needinfo?(dustin)
Attachment #8851925 - Flags: review?(dustin)
I'll have a look at the patch shortly.
Assignee: nobody → ayodeji.oyewole
Flags: needinfo?(dustin)
Comment on attachment 8851925 [details] [diff] [review]
Patch for bug 1351010

Review of attachment 8851925 [details] [diff] [review]:

Yes, this is definitely on the right track!  There are several optimization functions to update, and the code that calls the optimization will need to be updated, too.

I think this will be a good bug for learning the development workflow, too.  One suggestion I can make is to set up mozreview:
this makes it easier for you to submit review requests, and makes it much easier for reviewers to look at and comment on them.

Thanks for jumping into this bug!
Attachment #8851925 - Flags: review?(dustin) → feedback+
I made a new review request on mozreview, I hope you can see it. I am still learning how things work here, I hope I set up the review request correctly.

I fixed 3 functions in (opt_index_search, opt_seta, opt_files_changed). For those 3 functions I now have to fix every function that calls them.

The only optimize function I am yet to fix in is optimize_task(). If I change the return statement in optimize_task() to single values like you recommended, the function annotate_task_graph() in the same file would break. See @line 121.
Comment on attachment 8852323 [details]
Bug 1351010 - Fix return statements in;

Looking good so far -- keep it up!
Attachment #8852323 - Flags: review?(dustin)
Hi Dustin,

I added some more changes addressing this issue, I think it is fixed now.

Please have a look at my new commit on MozReview.

Flags: needinfo?(dustin)
You already added a review flag -- there's no need to add additional flags for me.
Flags: needinfo?(dustin)
Comment on attachment 8853693 [details]
Bug 1351010 - Completely fixed;

Looks good!  Can you merge these two commits together, since they are both parts of the same change?
Attachment #8853693 - Flags: review?(dustin) → review+
Comment on attachment 8853693 [details]
Bug 1351010 - Completely fixed;

I'm not sure I know how to do that, how do I merge the 2 commits?
It depends on which Mercurial extensions you have installed, but if you have the Histedit extension, just run `hg histedit` and then use the `fold` option to fold the second commit into the first.
Attachment #8853693 - Attachment is obsolete: true
Comment on attachment 8852323 [details]
Bug 1351010 - Fix return statements in;

Hi Dustin,

I have merged the 2 commits into this one commit. Please let me know the next steps to getting these changes landed.

Thank you.
Comment on attachment 8852323 [details]
Bug 1351010 - Fix return statements in;

::: commit-message-5182b:7
(Diff revision 2)
> +
> +MozReview-Commit-ID: ctgm1fw0Fo
> +***
> +Bug 1351010 - Completely fixed; r?dustin
> +
> +MozReview-Commit-ID: HKoWcINVSnV

The only complaint I have is here -- it would have been better to edit this down to a single message, rather than leave the "Completely fixed" message in there.  But it's not a big deal.  No more steps -- I'll get this merged!
Attachment #8852323 - Flags: review?(dustin) → review+
Whoops, this never did get merged.  Hmm.  I'll try again.
Pushed by
Fix return statements in; r=dustin
Sorry, I had to back this out for failing taskcluster/taskgraph/test/ (please also update the test and submit an updated patch):

Push with failure:
Failure log:

[task 2017-04-26T13:34:07.408414Z] TEST-PASS | /home/worker/checkouts/gecko/taskcluster/taskgraph/test/ | TestOptimize.test_annotate_task_graph_do_not_optimize
[task 2017-04-26T13:34:07.409781Z] TEST-UNEXPECTED-FAIL | /home/worker/checkouts/gecko/taskcluster/taskgraph/test/ | TestOptimize.test_annotate_task_graph_no_optimize, line 106: {u'task1': (True, (False, None)), u'task2': (True, (False, None)), u'task3': (Tr [truncated]... != {'task1': (False, None), 'task2': (False, None), 'task3': (False, None)}
[task 2017-04-26T13:34:07.409858Z] FAIL: annotating marks everything as un-optimized if the kind returns that
[task 2017-04-26T13:34:07.409907Z] Traceback (most recent call last):
[task 2017-04-26T13:34:07.409984Z]   File "/home/worker/checkouts/gecko/taskcluster/taskgraph/test/", line 106, in test_annotate_task_graph_no_optimize
[task 2017-04-26T13:34:07.410025Z]     task3=(False, None)
[task 2017-04-26T13:34:07.410098Z]   File "/home/worker/checkouts/gecko/taskcluster/taskgraph/test/", line 90, in assert_annotations
[task 2017-04-26T13:34:07.410153Z]     self.assertEqual(got_annotations, annotations)
[task 2017-04-26T13:34:07.410240Z] AssertionError: {u'task1': (True, (False, None)), u'task2': (True, (False, None)), u'task3': (Tr [truncated]... != {'task1': (False, None), 'task2': (False, None), 'task3': (False, None)}
[task 2017-04-26T13:34:07.410294Z] + {'task1': (False, None), 'task2': (False, None), 'task3': (False, None)}
[task 2017-04-26T13:34:07.410329Z] - {u'task1': (True, (False, None)),
[task 2017-04-26T13:34:07.410362Z] -  u'task2': (True, (False, None)),
[task 2017-04-26T13:34:07.410394Z] -  u'task3': (True, (False, None))}
[task 2017-04-26T13:34:07.410416Z] 
[task 2017-04-26T13:34:07.411644Z] TEST-UNEXPECTED-FAIL | /home/worker/checkouts/gecko/taskcluster/taskgraph/test/ | TestOptimize.test_annotate_task_graph_nos_do_not_propagate, line 161: {u'task1': (True, (False, None)), u'task2': (True, (True, u'taskid')), u'task3': [truncated]... != {'task1': (False, None), 'task2': (True, u'taskid'), 'task3': (True, u'taskid')} [truncated]...
[task 2017-04-26T13:34:07.411710Z] FAIL: a task with a non-optimized dependency can be optimized
[task 2017-04-26T13:34:07.411750Z] Traceback (most recent call last):
[task 2017-04-26T13:34:07.411820Z]   File "/home/worker/checkouts/gecko/taskcluster/taskgraph/test/", line 161, in test_annotate_task_graph_nos_do_not_propagate
[task 2017-04-26T13:34:07.411867Z]     task3=(True, 'taskid')
[task 2017-04-26T13:34:07.411930Z]   File "/home/worker/checkouts/gecko/taskcluster/taskgraph/test/", line 90, in assert_annotations
[task 2017-04-26T13:34:07.411980Z]     self.assertEqual(got_annotations, annotations)
[task 2017-04-26T13:34:07.412074Z] AssertionError: {u'task1': (True, (False, None)), u'task2': (True, (True, u'taskid')), u'task3': [truncated]... != {'task1': (False, None), 'task2': (True, u'taskid'), 'task3': (True, u'taskid')} [truncated]...
[task 2017-04-26T13:34:07.412125Z] - {u'task1': (True, (False, None)),
[task 2017-04-26T13:34:07.412168Z] ?  -         -------             -
[task 2017-04-26T13:34:07.412202Z] 
[task 2017-04-26T13:34:07.412233Z] + {'task1': (False, None),
[task 2017-04-26T13:34:07.412278Z] -  u'task2': (True, (True, u'taskid')),
[task 2017-04-26T13:34:07.412316Z] ?  -               -------           -
[task 2017-04-26T13:34:07.412342Z] 
[task 2017-04-26T13:34:07.412374Z] +  'task2': (True, u'taskid'),
[task 2017-04-26T13:34:07.412408Z] -  u'task3': (True, (True, u'taskid'))}
[task 2017-04-26T13:34:07.412443Z] ?  -               -------           -
[task 2017-04-26T13:34:07.412474Z] 
[task 2017-04-26T13:34:07.412506Z] +  'task3': (True, u'taskid')}
[task 2017-04-26T13:34:07.412534Z] 
[task 2017-04-26T13:34:07.412618Z] TEST-UNEXPECTED-FAIL | /home/worker/checkouts/gecko/taskcluster/taskgraph/test/ | TestOptimize.test_annotate_task_graph_optimize_away_dependency, line 126: Exception not raised
[task 2017-04-26T13:34:07.412676Z] FAIL: raises exception if kind optimizes away a task on which another depends
[task 2017-04-26T13:34:07.412715Z] Traceback (most recent call last):
[task 2017-04-26T13:34:07.412785Z]   File "/home/worker/checkouts/gecko/taskcluster/taskgraph/test/", line 126, in test_annotate_task_graph_optimize_away_dependency
[task 2017-04-26T13:34:07.412849Z]     lambda: annotate_task_graph(graph, {}, set(), graph.graph.named_links_dict(), {}, None)
[task 2017-04-26T13:34:07.412887Z] AssertionError: Exception not raised
[task 2017-04-26T13:34:07.412910Z] 
[task 2017-04-26T13:34:07.412990Z] TEST-UNEXPECTED-FAIL | /home/worker/checkouts/gecko/taskcluster/taskgraph/test/ | TestOptimize.test_annotate_task_graph_taskid_without_optimize, line 114: Exception not raised
[task 2017-04-26T13:34:07.413044Z] FAIL: raises exception if kind returns a taskid without optimizing
[task 2017-04-26T13:34:07.413080Z] Traceback (most recent call last):
[task 2017-04-26T13:34:07.413155Z]   File "/home/worker/checkouts/gecko/taskcluster/taskgraph/test/", line 114, in test_annotate_task_graph_taskid_without_optimize
[task 2017-04-26T13:34:07.413216Z]     lambda: annotate_task_graph(graph, {}, set(), graph.graph.named_links_dict(), {}, None)
[task 2017-04-26T13:34:07.413252Z] AssertionError: Exception not raised
[task 2017-04-26T13:34:07.413283Z] 
[task 2017-04-26T13:34:07.413348Z] TEST-PASS | /home/worker/checkouts/gecko/taskcluster/taskgraph/test/ | TestOptimize.test_get_subgraph_dep_chain
[task 2017-04-26T13:34:07.413422Z] TEST-PASS | /home/worker/checkouts/gecko/taskcluster/taskgraph/test/ | TestOptimize.test_get_subgraph_opt_away
[task 2017-04-26T13:34:07.413491Z] TEST-PASS | /home/worker/checkouts/gecko/taskcluster/taskgraph/test/ | TestOptimize.test_get_subgraph_refs_resolved
[task 2017-04-26T13:34:07.413694Z] TEST-PASS | /home/worker/checkouts/gecko/taskcluster/taskgraph/test/ | TestOptimize.test_get_subgraph_single_dep
[task 2017-04-26T13:34:07.414145Z] TEST-UNEXPECTED-FAIL | /home/worker/checkouts/gecko/taskcluster/taskgraph/test/ | TestOptimize.test_optimize, line 253: <Graph nodes=set([]) edges=set([])> != <Graph nodes=set([(False, None)]) edges=set([((False, None), (False, None), u'image')])>
[task 2017-04-26T13:34:07.414213Z] FAIL: optimize_task_graph annotates and extracts the subgraph from a simple graph
[task 2017-04-26T13:34:07.414261Z] Traceback (most recent call last):
[task 2017-04-26T13:34:07.414327Z]   File "/home/worker/checkouts/gecko/taskcluster/taskgraph/test/", line 253, in test_optimize
[task 2017-04-26T13:34:07.414383Z]     {(label_to_taskid['task2'], label_to_taskid['task3'], 'image')}))
[task 2017-04-26T13:34:07.414459Z] AssertionError: <Graph nodes=set([]) edges=set([])> != <Graph nodes=set([(False, None)]) edges=set([((False, None), (False, None), u'image')])>
[task 2017-04-26T13:34:07.414489Z]
Flags: needinfo?(ayodeji.oyewole)
That was my fault -- I shouldn't have r+'d it :(
I'll wrap this up.
Assignee: ayodeji.oyewole → dustin
I'd luck to claim this task.
(In reply to Amjad Mashaal from comment #21)
> I'd luck to claim this task.

like*. Sorry for the typo.
Assignee: dustin → me
Should be fixed. I've attached the file. Feel free to correct anything I did :)
Attachment #8865121 - Flags: review+
Attachment #8865121 - Attachment description: fix.patch → Fixes tests for bug 1351010
Attachment #8865121 - Flags: review+ → review?(dustin)
Comment on attachment 8865121 [details] [diff] [review]
Fixes tests for bug 1351010

Review of attachment 8865121 [details] [diff] [review]:

Looks great -- I'll get this landed.
Attachment #8865121 - Flags: review?(dustin) → review+
$ ./mach lint -l flake8
  110:5  error  too many blank lines (2)  E303 (flake8)

✖ 1 problem (1 error, 0 warnings)

I'll fix that up, but just FYI.
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Oops, through some failure of rebase I managed to land only the test change, and not the actual fix.  Which has now bitrotted.
Assignee: me → dustin
Flags: needinfo?(ayodeji.oyewole)
Resolution: FIXED → ---
Attachment #8868231 - Flags: review?(me)
Attachment #8868231 - Flags: review?(dustin)
Attachment #8868231 - Flags: review?(bstack)
Comment on attachment 8868231 [details]
Bug 1351010 - Return a single value from optimize functions

Looks like it makes sense!

::: taskcluster/taskgraph/
(Diff revision 2)
>          elif existing_tasks is not None and label in existing_tasks:
>              optimized = True
>              replacement_task_id = existing_tasks[label]
>          # otherwise, examine the task itself (which may be an expensive operation)
>          else:
> -            optimized, replacement_task_id = optimize_task(task, params)
> +            opt_result = optimize_task(task, params)

What about something like the following for lines 121-129

optimized = optimize_task(task, params)
replacement_task_id = optimized if (optimized and optimized is not True) else None

It feels a bit cleaner and less state to get into my head, but I'm only 75% sure that I make any sense here.
Attachment #8868231 - Flags: review?(bstack) → review+
Comment on attachment 8868231 [details]
Bug 1351010 - Return a single value from optimize functions
Attachment #8868231 - Flags: review?(me) → review+
Pushed by
Return a single value from optimize functions; r=bstack,TheNavigat
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Depends on: 1369630
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.