fix: replaced taskcluster rest api calls with package #741

abhishekmadan30 · 2025-08-12T18:40:40Z

fixes #221 . Created a bunch of helpers to use this package in taskcluster.py . I also didnt completely depcreate the url creation as that is still used. I bypassed it and replaced all uses when to get request is made.

ahal

This is shaping up nicely, thanks!

I have some changes to request. Once you put up a new revision I'll pull this down locally and see if it causes any differences in the graph (both here and in Gecko).

src/taskgraph/run-task/fetch-content

src/taskgraph/util/taskcluster.py

taskcluster/scripts/codecov-upload.py

fixes taskcluster#221

This reverts commit cfb77a0.

ahal · 2025-08-19T20:47:36Z

Thanks! I'll take a closer look at this tomorrow. I'll try and test it against gecko_taskgraph as well.

ahal

Thanks, I tested this patch both in Taskgraph and the Gecko repo and it looks like it's working as expected!

But get_retry_post_session, _do_request and _handle_artifact are all unused now in taskcluster.py. Can you remove these and associated tests please?

ahal · 2025-08-20T19:01:47Z

Also I think I changed my mind about leaving use_proxy in the function signature just for backwards compatibility.. We should remove it rather than confuse consumers who are attempting to set it to False or whatnot and then wondering why it isn't working.

We can do a major version bump for this.. no big deal.

ahal

Thank you!

This is mostly cleanup, but there are two issues I neglected to catch in my first review:

The _handle_artifact for yaml
The .capitalize() issue

ahal · 2025-08-22T19:28:14Z

src/taskgraph/util/taskcluster.py

-    response.raw.read = functools.partial(response.raw.read, decode_content=True)
-    return response.raw
-
-
 def get_artifact_url(task_id, path, use_proxy=False):


I think we should leave this for a follow-up issue, but you can actually get artifact urls and the like from the Taskcluster client as well, using something like:

queue.buildUrl("getArtifact", task_id, path)

If we used this in get_artifact_url, get_index_url and get_task_url, I think we could get rid of the get_root_url method as well as the dependency on taskcluster_urls.

But as I mentioned, this is close to landing, so let's get this landed and maybe consider this change later.

ahal · 2025-08-22T19:40:56Z

src/taskgraph/util/taskcluster.py

-    return _handle_artifact(path, response)
+    queue = get_taskcluster_client("queue")
+    response = queue.getArtifact(task_id, path)
+    return response


This could cause some problems, at least if I'm reading this right, we used to load yaml artifacts and now we don't. I think the taskcluster client will handle json for us, but not yaml.

So maybe we can add the _handle_artifact logic back in? Sorry, I said to remove it but I was focused on the fact that it was unused and didn't notice the regression.

ahal · 2025-08-22T19:43:40Z

src/taskgraph/util/taskcluster.py

-            break
+    tasks = response.get("tasks", [])
+    for task in tasks:
+        results.append(task)


This loop doesn't seem to do anything

ahal · 2025-08-22T19:46:19Z

src/taskgraph/util/taskcluster.py

+        if "taskId" in t:
+            task_id = t.get("taskId")
+            if task_id:
+                task_ids.append(task_id)


There's some superfluous existence checks here. You could re-write this as:

for t in results: if task_id := t.get("taskId"): task_ids.append(task_id)

(look up the "walrus operator" if that confuses you)

But this can be further simplified using a list comprehension:

task_ids = [t["taskId"] for t in results if "taskId" in t]

ahal · 2025-08-22T19:48:49Z

src/taskgraph/util/taskcluster.py

+        namespace = task.get("namespace")  # type: ignore
+        task_id = task.get("taskId")  # type: ignore
+        if namespace and task_id:
+            task_ids[namespace] = task_id


This can be re-written using a dictionary comprehension:

task_ids = { t["namespace"]: t["taskId"] for t in tasks if "namespace" in t and "taskId" in t }

ahal · 2025-08-22T19:55:29Z

src/taskgraph/util/taskcluster.py

+    else:
+        options = taskcluster.optionsFromEnvironment()
+
+    return getattr(taskcluster, service.capitalize())(options)


I realized the use of capitalize here is going to break the purge_cache function (and also WorkerManager, but we don't use that here)..

E.g:

>>> "purgeCache".capitalize() 'Purgecache'

I know you copied this code from something I wrote, so my bad.. Maybe instead of using .capitalize() we can do something like service[0].upper() + service[1:]

ahal · 2025-08-22T19:57:05Z

src/taskgraph/util/taskcluster.py

    """Requests a cache purge from the purge-caches service."""
    if testing:
        logger.info(f"Would have purged {provisioner_id}/{worker_type}/{cache_name}.")
    else:
        logger.info(f"Purging {provisioner_id}/{worker_type}/{cache_name}.")
-        purge_cache_url = get_purge_cache_url(provisioner_id, worker_type, use_proxy)
-        _do_request(purge_cache_url, json={"cacheName": cache_name})
+        purge_cache_client = get_taskcluster_client("purgeCache")


(see my note about this above)

ahal · 2025-08-22T19:57:41Z

src/taskgraph/util/taskcluster.py

+    tasks = response.get("tasks", [])
+    for task in tasks:
+        if "status" in task:  # type: ignore
+            status = task["status"]  # type: ignore


Good opportunity to try out the walrus operator ;)

ahal · 2025-08-22T20:01:53Z

src/taskgraph/util/taskcluster.py

-                continue
-
-            raise e
+            continue


Are you sure the taskcluster client would return None on a 404? I would have thought it would raise an exception still.. Though it would probably be from aiohttp rather than requests.

ahal · 2025-08-22T20:03:19Z

test/test_optimize_strategies.py

@@ -57,29 +58,31 @@ def params():
        ),
    ),
 )
-def test_index_search(caplog, responses, params, state, expires, expected, logs):


This is fine, but fyi there exists an aioresponses mock library designed to be used with aiohttp that we could make use of in a similar way.

abhishekmadan30 requested a review from a team as a code owner August 12, 2025 18:40

abhishekmadan30 requested a review from jcristau August 12, 2025 18:40

abhishekmadan30 force-pushed the issue-221 branch from ec75ed6 to 40c0435 Compare August 12, 2025 18:59

ahal requested changes Aug 12, 2025

View reviewed changes

fix: replaced taskcluster rest api calls with package

80451af

fixes taskcluster#221

abhishekmadan30 force-pushed the issue-221 branch from 40c0435 to 80451af Compare August 13, 2025 15:38

abhishekmadan30 and others added 5 commits August 18, 2025 14:16

fix: replaced taskcluster rest api calls with package

cfb77a0

fixes taskcluster#221

Revert "fix: replaced taskcluster rest api calls with package"

ce59615

This reverts commit cfb77a0.

fix: converted rest api to python package

384afee

fix: convert to taskcluster package

87a0ecc

Merge branch 'main' into issue-221

1dda67a

abhishekmadan30 requested a review from ahal August 19, 2025 19:45

Merge branch 'main' into issue-221

afef6ba

ahal requested changes Aug 20, 2025

View reviewed changes

fix: removed unused functions and use_proxy

2e7e853

abhishekmadan30 force-pushed the issue-221 branch from 826551a to 2e7e853 Compare August 21, 2025 20:51

abhishekmadan30 requested a review from ahal August 22, 2025 18:13

ahal requested changes Aug 22, 2025

View reviewed changes

fix: replaced taskcluster rest api calls with package #741

Are you sure you want to change the base?

fix: replaced taskcluster rest api calls with package #741

Uh oh!

Conversation

abhishekmadan30 commented Aug 12, 2025

Uh oh!

ahal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahal commented Aug 19, 2025

Uh oh!

ahal left a comment

Choose a reason for hiding this comment

Uh oh!

ahal commented Aug 20, 2025

Uh oh!

ahal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!