From 3482b038216544070df78a8ce5c82939455c8c7f Mon Sep 17 00:00:00 2001 From: YaySushi Date: Sat, 31 Aug 2024 17:32:11 -0400 Subject: [PATCH 1/3] Fix some type checking errors after removing type hint ignores --- src/taskgraph/actions/cancel.py | 2 +- src/taskgraph/actions/cancel_all.py | 2 +- src/taskgraph/actions/rebuild_cached_tasks.py | 2 +- src/taskgraph/actions/util.py | 6 +- src/taskgraph/generator.py | 10 +-- src/taskgraph/main.py | 11 ++-- src/taskgraph/parameters.py | 4 +- src/taskgraph/run-task/robustcheckout.py | 17 ++--- src/taskgraph/util/vcs.py | 62 +++++++++++++------ 9 files changed, 73 insertions(+), 43 deletions(-) diff --git a/src/taskgraph/actions/cancel.py b/src/taskgraph/actions/cancel.py index 320e96087..33a5394e6 100644 --- a/src/taskgraph/actions/cancel.py +++ b/src/taskgraph/actions/cancel.py @@ -29,7 +29,7 @@ def cancel_action(parameters, graph_config, input, task_group_id, task_id): try: cancel_task(task_id, use_proxy=True) except requests.HTTPError as e: - if e.response.status_code == 409: # type: ignore + if e.response.status_code == 409: # A 409 response indicates that this task is past its deadline. It # cannot be cancelled at this time, but it's also not running # anymore, so we can ignore this error. diff --git a/src/taskgraph/actions/cancel_all.py b/src/taskgraph/actions/cancel_all.py index c17baa031..55453b762 100644 --- a/src/taskgraph/actions/cancel_all.py +++ b/src/taskgraph/actions/cancel_all.py @@ -38,7 +38,7 @@ def do_cancel_task(task_id): try: cancel_task(task_id, use_proxy=True) except requests.HTTPError as e: - if e.response.status_code == 409: # type: ignore + if e.response.status_code == 409: # A 409 response indicates that this task is past its deadline. It # cannot be cancelled at this time, but it's also not running # anymore, so we can ignore this error. diff --git a/src/taskgraph/actions/rebuild_cached_tasks.py b/src/taskgraph/actions/rebuild_cached_tasks.py index 3e538778c..8ea2e3715 100644 --- a/src/taskgraph/actions/rebuild_cached_tasks.py +++ b/src/taskgraph/actions/rebuild_cached_tasks.py @@ -22,7 +22,7 @@ def rebuild_cached_tasks_action( ) cached_tasks = [ label - for label, task in full_task_graph.tasks.items() # type: ignore + for label, task in full_task_graph.tasks.items() if task.attributes.get("cached_task", False) ] if cached_tasks: diff --git a/src/taskgraph/actions/util.py b/src/taskgraph/actions/util.py index d356e1267..13c0c1278 100644 --- a/src/taskgraph/actions/util.py +++ b/src/taskgraph/actions/util.py @@ -60,7 +60,7 @@ def fetch_action(task_id): run_label_to_id = get_artifact(task_id, "public/label-to-taskid.json") label_to_taskid.update(run_label_to_id) except HTTPError as e: - if e.response.status_code != 404: # type: ignore + if e.response.status_code != 404: raise logger.debug(f"No label-to-taskid.json found for {task_id}: {e}") @@ -79,7 +79,7 @@ def fetch_cron(task_id): run_label_to_id = get_artifact(task_id, "public/label-to-taskid.json") label_to_taskid.update(run_label_to_id) except HTTPError as e: - if e.response.status_code != 404: # type: ignore + if e.response.status_code != 404: raise logger.debug(f"No label-to-taskid.json found for {task_id}: {e}") @@ -161,7 +161,7 @@ def create_tasks( target_graph = full_task_graph.graph.transitive_closure(to_run) target_task_graph = TaskGraph( {l: modifier(full_task_graph[l]) for l in target_graph.nodes}, - target_graph, # type: ignore + target_graph, ) target_task_graph.for_each_task(update_parent) if decision_task_id and decision_task_id != os.environ.get("TASK_ID"): diff --git a/src/taskgraph/generator.py b/src/taskgraph/generator.py index 9da14e746..4be841eb3 100644 --- a/src/taskgraph/generator.py +++ b/src/taskgraph/generator.py @@ -295,7 +295,7 @@ def _run(self): for kind in kinds.values(): for dep in kind.config.get("kind-dependencies", []): edges.add((kind.name, dep, "kind-dependency")) - kind_graph = Graph(set(kinds), edges) # type: ignore + kind_graph = Graph(frozenset(kinds), frozenset(edges)) if target_kinds: kind_graph = kind_graph.transitive_closure( @@ -321,7 +321,7 @@ def _run(self): raise Exception("duplicate tasks with label " + task.label) all_tasks[task.label] = task logger.info(f"Generated {len(new_tasks)} tasks for kind {kind_name}") - full_task_set = TaskGraph(all_tasks, Graph(set(all_tasks), set())) # type: ignore + full_task_set = TaskGraph(all_tasks, Graph(frozenset(all_tasks), frozenset())) yield self.verify("full_task_set", full_task_set, graph_config, parameters) logger.info("Generating full task graph") @@ -334,7 +334,7 @@ def _run(self): ) edges.add((t.label, dep, depname)) - full_task_graph = TaskGraph(all_tasks, Graph(full_task_set.graph.nodes, edges)) # type: ignore + full_task_graph = TaskGraph(all_tasks, Graph(frozenset(full_task_set.graph.nodes), frozenset(edges))) logger.info( "Full task graph contains %d tasks and %d dependencies" % (len(full_task_set.graph.nodes), len(edges)) @@ -344,14 +344,14 @@ def _run(self): logger.info("Generating target task set") target_task_set = TaskGraph( dict(all_tasks), - Graph(set(all_tasks.keys()), set()), # type: ignore + Graph(frozenset(all_tasks.keys()), frozenset()), ) for fltr in filters: old_len = len(target_task_set.graph.nodes) target_tasks = set(fltr(target_task_set, parameters, graph_config)) target_task_set = TaskGraph( {l: all_tasks[l] for l in target_tasks}, - Graph(target_tasks, set()), # type: ignore + Graph(frozenset(target_tasks), frozenset()), ) logger.info( "Filter %s pruned %d tasks (%d remain)" diff --git a/src/taskgraph/main.py b/src/taskgraph/main.py index 1a2960bbb..2540a233f 100644 --- a/src/taskgraph/main.py +++ b/src/taskgraph/main.py @@ -88,10 +88,10 @@ def get_filtered_taskgraph(taskgraph, tasksregex, exclude_keys): if regexprogram.match(dep): filterededges.add((key, dep, depname)) - taskgraph = TaskGraph(filteredtasks, Graph(set(filteredtasks), filterededges)) # type: ignore + taskgraph = TaskGraph(filteredtasks, Graph(frozenset(filteredtasks), frozenset(filterededges))) if exclude_keys: - for label, task in taskgraph.tasks.items(): # type: ignore + for label, task in taskgraph.tasks.items(): task = task.to_json() for key in exclude_keys: obj = task @@ -862,7 +862,10 @@ def init_taskgraph(options): context = {"project_name": root.name, "taskgraph_version": taskgraph.__version__} try: - repo_url = repo.get_url(remote=repo.remote_name) # type: ignore + if isinstance(repo.remote_name, str): + repo_url = repo.get_url(remote=repo.remote_name) + else: + repo_url = "" except RuntimeError: repo_url = "" @@ -895,7 +898,7 @@ def init_taskgraph(options): directory="template", extra_context=context, no_input=options["no_input"], - output_dir=root.parent, # type: ignore + output_dir=str(root.parent), overwrite_if_exists=True, ) diff --git a/src/taskgraph/parameters.py b/src/taskgraph/parameters.py index 0781a5f6b..c322eeba2 100644 --- a/src/taskgraph/parameters.py +++ b/src/taskgraph/parameters.py @@ -95,8 +95,8 @@ def _get_defaults(repo_root=None): project = parsed_url.repo_name except ( CalledProcessError, - mozilla_repo_urls.errors.InvalidRepoUrlError, # type: ignore - mozilla_repo_urls.errors.UnsupportedPlatformError, # type: ignore + mozilla_repo_urls.InvalidRepoUrlError, + mozilla_repo_urls.UnsupportedPlatformError, ): repo_url = "" project = "" diff --git a/src/taskgraph/run-task/robustcheckout.py b/src/taskgraph/run-task/robustcheckout.py index 5ec0c90c5..fd1a17546 100644 --- a/src/taskgraph/run-task/robustcheckout.py +++ b/src/taskgraph/run-task/robustcheckout.py @@ -337,6 +337,7 @@ def callself(): @contextlib.contextmanager def timeit(op, behavior): behaviors.add(behavior) + start = 0 errored = False try: start = time.time() @@ -345,7 +346,7 @@ def timeit(op, behavior): errored = True raise finally: - elapsed = time.time() - start # type: ignore + elapsed = time.time() - start if errored: op += "_errored" @@ -672,6 +673,7 @@ def handlepullerror(e): # We only pull if we are using symbolic names or the requested revision # doesn't exist. havewantedrev = False + checkoutrevision = None if revision: try: @@ -748,6 +750,7 @@ def handlepullerror(e): # Mercurial 4.3 doesn't purge files outside the sparse checkout. # See https://bz.mercurial-scm.org/show_bug.cgi?id=5626. Force # purging by monkeypatching the sparse matcher. + old_sparse_fn = None try: old_sparse_fn = getattr(repo.dirstate, "_sparsematchfn", None) if old_sparse_fn is not None: @@ -765,8 +768,8 @@ def handlepullerror(e): ): raise error.Abort(b"error purging") finally: - if old_sparse_fn is not None: # type: ignore - repo.dirstate._sparsematchfn = old_sparse_fn # type: ignore + if old_sparse_fn is not None: + repo.dirstate._sparsematchfn = old_sparse_fn # Update the working directory. @@ -781,11 +784,11 @@ def handlepullerror(e): # By default, Mercurial will ignore unknown sparse profiles. This could # lead to a full checkout. Be more strict. try: - repo.filectx(sparse_profile, changeid=checkoutrevision).data() # type: ignore + repo.filectx(sparse_profile, changeid=checkoutrevision).data() except error.ManifestLookupError: raise error.Abort( b"sparse profile %s does not exist at revision " - b"%s" % (sparse_profile, checkoutrevision) # type: ignore + b"%s" % (sparse_profile, checkoutrevision) ) old_config = sparsemod.parseconfig( @@ -843,10 +846,10 @@ def parentchange(repo): behavior = "update-sparse" if sparse_profile else "update" with timeit(op, behavior): - if commands.update(ui, repo, rev=checkoutrevision, clean=True): # type: ignore + if commands.update(ui, repo, rev=checkoutrevision, clean=True): raise error.Abort(b"error updating") - ui.write(b"updated to %s\n" % checkoutrevision) # type: ignore + ui.write(b"updated to %s\n" % checkoutrevision) return None diff --git a/src/taskgraph/util/vcs.py b/src/taskgraph/util/vcs.py index 444bc815d..fa0b5cd2d 100644 --- a/src/taskgraph/util/vcs.py +++ b/src/taskgraph/util/vcs.py @@ -7,8 +7,9 @@ import os import re import subprocess -from abc import ABC, abstractmethod, abstractproperty +from abc import ABC, abstractmethod from shutil import which +from collections.abc import Sized from taskgraph.util.path import ancestors @@ -51,45 +52,57 @@ def run(self, *args: str, **kwargs): return "" raise - @abstractproperty - def tool(self) -> str: # type: ignore + @property + @abstractmethod + def tool(self) -> str: """Version control system being used, either 'hg' or 'git'.""" - @abstractproperty - def head_rev(self) -> str: # type: ignore + @property + @abstractmethod + def head_rev(self) -> str: """Hash of HEAD revision.""" - @abstractproperty + @property + @abstractmethod def base_rev(self): """Hash of revision the current topic branch is based on.""" - @abstractproperty + @property + @abstractmethod def branch(self): """Current branch or bookmark the checkout has active.""" - @abstractproperty + @property + @abstractmethod def all_remote_names(self): """Name of all configured remote repositories.""" - @abstractproperty + @property + @abstractmethod def default_remote_name(self): """Name the VCS defines for the remote repository when cloning it for the first time. This name may not exist anymore if users changed the default configuration, for instance.""" - @abstractproperty + @property + @abstractmethod def remote_name(self): """Name of the remote repository.""" def _get_most_suitable_remote(self, remote_instructions): remotes = self.all_remote_names - if len(remotes) == 1: # type: ignore - return remotes[0] # type: ignore - if self.default_remote_name in remotes: # type: ignore + # in case all_remote_names raised a RuntimeError + if remotes is None: + raise RuntimeError("No valid remotes found") + + if len(remotes) == 1: + return remotes[0] + + if self.default_remote_name in remotes: return self.default_remote_name - first_remote = remotes[0] # type: ignore + first_remote = remotes[0] logger.warning( f"Unable to determine which remote repository to use between: {remotes}. " f'Arbitrarily using the first one "{first_remote}". Please set an ' @@ -99,7 +112,8 @@ def _get_most_suitable_remote(self, remote_instructions): return first_remote - @abstractproperty + @property + @abstractmethod def default_branch(self): """Name of the default branch.""" @@ -181,8 +195,13 @@ def does_revision_exist_locally(self, revision): class HgRepository(Repository): - tool = "hg" # type: ignore - default_remote_name = "default" # type: ignore + @property + def tool(self) -> str: + return "hg" + + @property + def default_remote_name(self) -> str: + return "default" def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) @@ -321,8 +340,13 @@ def does_revision_exist_locally(self, revision): class GitRepository(Repository): - tool = "git" # type: ignore - default_remote_name = "origin" # type: ignore + @property + def tool(self) -> str: + return "git" + + @property + def default_remote_name(self) -> str: + return "origin" _LS_REMOTE_PATTERN = re.compile(r"ref:\s+refs/heads/(?P\S+)\s+HEAD") From eb40dce9f88f94ab0fb2d6965ba15f2e096bfd1c Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 31 Aug 2024 21:48:50 +0000 Subject: [PATCH 2/3] style: pre-commit.ci auto fixes [...] --- src/taskgraph/generator.py | 4 +++- src/taskgraph/main.py | 4 +++- src/taskgraph/util/vcs.py | 1 - 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/taskgraph/generator.py b/src/taskgraph/generator.py index 4be841eb3..0e6063296 100644 --- a/src/taskgraph/generator.py +++ b/src/taskgraph/generator.py @@ -334,7 +334,9 @@ def _run(self): ) edges.add((t.label, dep, depname)) - full_task_graph = TaskGraph(all_tasks, Graph(frozenset(full_task_set.graph.nodes), frozenset(edges))) + full_task_graph = TaskGraph( + all_tasks, Graph(frozenset(full_task_set.graph.nodes), frozenset(edges)) + ) logger.info( "Full task graph contains %d tasks and %d dependencies" % (len(full_task_set.graph.nodes), len(edges)) diff --git a/src/taskgraph/main.py b/src/taskgraph/main.py index 2540a233f..94e306418 100644 --- a/src/taskgraph/main.py +++ b/src/taskgraph/main.py @@ -88,7 +88,9 @@ def get_filtered_taskgraph(taskgraph, tasksregex, exclude_keys): if regexprogram.match(dep): filterededges.add((key, dep, depname)) - taskgraph = TaskGraph(filteredtasks, Graph(frozenset(filteredtasks), frozenset(filterededges))) + taskgraph = TaskGraph( + filteredtasks, Graph(frozenset(filteredtasks), frozenset(filterededges)) + ) if exclude_keys: for label, task in taskgraph.tasks.items(): diff --git a/src/taskgraph/util/vcs.py b/src/taskgraph/util/vcs.py index fa0b5cd2d..9fc3c1072 100644 --- a/src/taskgraph/util/vcs.py +++ b/src/taskgraph/util/vcs.py @@ -9,7 +9,6 @@ import subprocess from abc import ABC, abstractmethod from shutil import which -from collections.abc import Sized from taskgraph.util.path import ancestors From ee5cae7d5c20deaddec23ff5296e249e171c179b Mon Sep 17 00:00:00 2001 From: YaySushi Date: Tue, 3 Sep 2024 15:47:37 -0400 Subject: [PATCH 3/3] Address review comments --- src/taskgraph/main.py | 5 +---- src/taskgraph/optimize/base.py | 5 +++-- src/taskgraph/util/vcs.py | 2 +- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/taskgraph/main.py b/src/taskgraph/main.py index 2540a233f..b12577229 100644 --- a/src/taskgraph/main.py +++ b/src/taskgraph/main.py @@ -862,10 +862,7 @@ def init_taskgraph(options): context = {"project_name": root.name, "taskgraph_version": taskgraph.__version__} try: - if isinstance(repo.remote_name, str): - repo_url = repo.get_url(remote=repo.remote_name) - else: - repo_url = "" + repo_url = repo.get_url(remote=repo.remote_name) except RuntimeError: repo_url = "" diff --git a/src/taskgraph/optimize/base.py b/src/taskgraph/optimize/base.py index adc354454..c40b9a87a 100644 --- a/src/taskgraph/optimize/base.py +++ b/src/taskgraph/optimize/base.py @@ -13,7 +13,7 @@ import datetime import logging -from abc import ABCMeta, abstractmethod, abstractproperty +from abc import ABCMeta, abstractmethod from collections import defaultdict from slugid import nice as slugid @@ -495,7 +495,8 @@ def __init__(self, *substrategies, **kwargs): if kwargs: raise TypeError("unexpected keyword args") - @abstractproperty + @property + @abstractmethod def description(self): """A textual description of the combined substrategies.""" diff --git a/src/taskgraph/util/vcs.py b/src/taskgraph/util/vcs.py index fa0b5cd2d..1050f596f 100644 --- a/src/taskgraph/util/vcs.py +++ b/src/taskgraph/util/vcs.py @@ -79,7 +79,7 @@ def all_remote_names(self): @property @abstractmethod - def default_remote_name(self): + def default_remote_name(self) -> str: """Name the VCS defines for the remote repository when cloning it for the first time. This name may not exist anymore if users changed the default configuration, for instance."""