Optimization functions look like this: @optimization('index-search') def opt_index_search(task, params, index_path): try: task_id = find_task_id( index_path, use_proxy=bool(os.environ.get('TASK_ID'))) return True, task_id except requests.exceptions.HTTPError: pass 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 optimize.py I assume some of the other functions in optimize.py 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. Thanks!

I'll have a look at the patch shortly.

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: http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview-user.html#mozreview-user 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!

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 optimize.py (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 optimize.py 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 optimize.py; https://reviewboard.mozilla.org/r/124576/#review127228 Looking good so far -- keep it up!

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

You already added a review flag -- there's no need to add additional flags for me.

Comment on attachment 8853693 [details] Bug 1351010 - Completely fixed; https://reviewboard.mozilla.org/r/125770/#review128664 Looks good! Can you merge these two commits together, since they are both parts of the same change?

Comment on attachment 8853693 [details] Bug 1351010 - Completely fixed; https://reviewboard.mozilla.org/r/125770/#review128664 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.

Comment on attachment 8852323 [details] Bug 1351010 - Fix return statements in optimize.py; https://reviewboard.mozilla.org/r/124576/#review127228 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 optimize.py; https://reviewboard.mozilla.org/r/124576/#review129436 ::: 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!

Whoops, this never did get merged. Hmm. I'll try again.

Pushed by dmitchell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e5073b47f5c8 Fix return statements in optimize.py; r=dustin

Sorry, I had to back this out for failing taskcluster/taskgraph/test/test_optimize.py (please also update the test and submit an updated patch): https://hg.mozilla.org/integration/autoland/rev/165d2a346e107a9d2837ddba10e60294aec71092 Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=e5073b47f5c8f7aa8fc2e5578d4fcc6a6c79deed&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=94504334&repo=autoland [task 2017-04-26T13:34:07.408414Z] TEST-PASS | /home/worker/checkouts/gecko/taskcluster/taskgraph/test/test_optimize.py | 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/test_optimize.py | 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/test_optimize.py", 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/test_optimize.py", 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/test_optimize.py | 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/test_optimize.py", 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/test_optimize.py", 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/test_optimize.py | 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/test_optimize.py", 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/test_optimize.py | 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/test_optimize.py", 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/test_optimize.py | TestOptimize.test_get_subgraph_dep_chain [task 2017-04-26T13:34:07.413422Z] TEST-PASS | /home/worker/checkouts/gecko/taskcluster/taskgraph/test/test_optimize.py | TestOptimize.test_get_subgraph_opt_away [task 2017-04-26T13:34:07.413491Z] TEST-PASS | /home/worker/checkouts/gecko/taskcluster/taskgraph/test/test_optimize.py | TestOptimize.test_get_subgraph_refs_resolved [task 2017-04-26T13:34:07.413694Z] TEST-PASS | /home/worker/checkouts/gecko/taskcluster/taskgraph/test/test_optimize.py | TestOptimize.test_get_subgraph_single_dep [task 2017-04-26T13:34:07.414145Z] TEST-UNEXPECTED-FAIL | /home/worker/checkouts/gecko/taskcluster/taskgraph/test/test_optimize.py | 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/test_optimize.py", 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]

That was my fault -- I shouldn't have r+'d it :(

I'll wrap this up.

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.

Should be fixed. I've attached the file. Feel free to correct anything I did :)

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.

$ ./mach lint -l flake8 /home/dustin/p/m-c/taskcluster/taskgraph/test/test_optimize.py 110:5 error too many blank lines (2) E303 (flake8) ✖ 1 problem (1 error, 0 warnings) I'll fix that up, but just FYI.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c163d3e47425a81beb6755dce4ee14f3d9004c Bug 1351010: Return a single value from optimize, with fixed tests; r=me

Oops, through some failure of rebase I managed to land only the test change, and not the actual fix. Which has now bitrotted.

Comment on attachment 8868231 [details] Bug 1351010 - Return a single value from optimize functions https://reviewboard.mozilla.org/r/139804/#review143562 Looks like it makes sense! ::: taskcluster/taskgraph/optimize.py:121 (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.

Comment on attachment 8868231 [details] Bug 1351010 - Return a single value from optimize functions https://reviewboard.mozilla.org/r/139804/#review144602

Pushed by dmitchell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3f28322e51c5 Return a single value from optimize functions; r=bstack,TheNavigat