Python warning + code cleanup (#4981)

* Enable strict type checking + python code cleanup

* Only import typing_extensions when type checking

* Fix dotnet version regex

* Address PR feedback

* Revert get_msbuild_property, add pyrightconfig.json

* Fix tracer errors

Author: caaavik-msft

.vscode/settings.json

@@ -1,15 +1,19 @@
 {
     "files.exclude": {
-        "**/__pycache__/": true,
-        "**/BenchmarkDotNet.Artifacts/": true,
-        "**/.vs/": true,
-        "**/artifacts/": true,
-        "**/bin/": true,
-        "**/logs/": true,
-        "**/obj/": true,
-        "**/packages/": true,
-        "/tools/": true,
+        "**/__pycache__/**": true,
+        "**/.vs/**": true,
+        "**/bin/**": true,
+        "**/obj/**": true,
+        "**/packages/**": true,
+        "**/.venv/**": true,
+        "**/.dotnet/**": true,
     },
-    "python.analysis.typeCheckingMode": "standard",
-    "python.analysis.extraPaths": ["scripts"]
+    "files.readonlyInclude": {
+        "/eng/common/**": true,
+    },
+    "search.exclude": {
+        "**/artifacts/**": true,
+        "**/weblarge3.0/**": true,
+    },
+    "python.analysis.diagnosticMode": "workspace"
 }

pyrightconfig.json

@@ -0,0 +1,94 @@
+{
+    "typeCheckingMode": "strict",
+    "include": ["src/scenarios", "scripts"],
+    "exclude": [
+        "**/__pycache__/**",
+        "**/artifacts/**",
+        "**/.venv/**",
+        "**/eng/common/**",
+        "**/weblarge3.0/**"
+    ],
+    "pythonVersion": "3.9",
+    "pythonPlatform": "All",
+    "deprecateTypingAliases": true,
+    "reportMissingTypeStubs": "none",
+    "reportImplicitOverride": "error",
+    "reportImportCycles": "error",
+    "reportPropertyTypeMismatch": "error",
+    "reportUnnecessaryTypeIgnoreComment": "error",
+    "reportUnreachable": "information",
+    // Ensure that src/scenarios and scripts are on the path by default for all execution environments
+    "extraPaths": ["./src/scenarios", "./scripts"],
+    "executionEnvironments": [
+        // scripts/tests run relative to the root of the repository
+        {
+            "root": "./scripts/tests",
+            "extraPaths": [ "." ]
+        },
+        // Set extra paths to empty as we don't want to accidentally reference src/scenarios from here
+        {
+            "root": "./scripts",
+            "extraPaths": [ ]
+        },
+        // All of our scenarios are run by executing python files pre.py, test.py, and post.py from their own directory
+        // This means we need to mark each scenario folder as a separate execution environment so that pyright is able
+        // to properly resolve relative imports inside each scenario folder.
+        { "root": "./src/scenarios/aspwebtemplate" },
+        { "root": "./src/scenarios/bdnandroid" },
+        { "root": "./src/scenarios/blazor" },
+        { "root": "./src/scenarios/blazoraot" },
+        { "root": "./src/scenarios/blazorlocalized" },
+        { "root": "./src/scenarios/blazorminapp" },
+        { "root": "./src/scenarios/blazorminappaot" },
+        { "root": "./src/scenarios/blazorpizza" },
+        { "root": "./src/scenarios/blazorpizzaaot" },
+        { "root": "./src/scenarios/blazorserverinnerloop" },
+        { "root": "./src/scenarios/blazorservertemplate" },
+        { "root": "./src/scenarios/blazorwasmdotnetwatch" },
+        { "root": "./src/scenarios/blazorwasminnerloop" },
+        { "root": "./src/scenarios/classlibtemplate" },
+        { "root": "./src/scenarios/crossgen" },
+        { "root": "./src/scenarios/crossgen2" },
+        { "root": "./src/scenarios/emptyconsolenativeaot" },
+        { "root": "./src/scenarios/emptyconsoletemplate" },
+        { "root": "./src/scenarios/emptyconsoletemplateinnerloop" },
+        { "root": "./src/scenarios/emptyconsoletemplateinnerloopmsbuild" },
+        { "root": "./src/scenarios/emptyfsconsoletemplate" },
+        { "root": "./src/scenarios/emptyvbconsoletemplate" },
+        { "root": "./src/scenarios/fsharpcompilerservice" },
+        { "root": "./src/scenarios/genericandroidstartup" },
+        { "root": "./src/scenarios/grpctemplate" },
+        { "root": "./src/scenarios/helloandroid" },
+        { "root": "./src/scenarios/helloios" },
+        { "root": "./src/scenarios/mauiandroid" },
+        { "root": "./src/scenarios/mauiblazorandroid" },
+        { "root": "./src/scenarios/mauiblazordesktop" },
+        { "root": "./src/scenarios/mauiblazorios" },
+        { "root": "./src/scenarios/mauidesktop" },
+        { "root": "./src/scenarios/mauiios" },
+        { "root": "./src/scenarios/mauimaccatalyst" },
+        { "root": "./src/scenarios/mauisamplecontentandroid" },
+        { "root": "./src/scenarios/mstesttemplate" },
+        { "root": "./src/scenarios/mvcapptemplate" },
+        { "root": "./src/scenarios/mvcdotnetwatch" },
+        { "root": "./src/scenarios/mvcinnerloop" },
+        { "root": "./src/scenarios/netandroid" },
+        { "root": "./src/scenarios/netios" },
+        { "root": "./src/scenarios/netstandard2.0" },
+        { "root": "./src/scenarios/nunittesttemplate" },
+        { "root": "./src/scenarios/paintdotnet" },
+        { "root": "./src/scenarios/razorclasslibtemplate" },
+        { "root": "./src/scenarios/staticconsoletemplate" },
+        { "root": "./src/scenarios/staticfsconsoletemplate" },
+        { "root": "./src/scenarios/staticvbconsoletemplate" },
+        { "root": "./src/scenarios/staticwinformstemplate" },
+        { "root": "./src/scenarios/webapitemplate" },
+        { "root": "./src/scenarios/webapptemplate" },
+        { "root": "./src/scenarios/weblarge3.0" },
+        { "root": "./src/scenarios/windowsforms" },
+        { "root": "./src/scenarios/windowsformslarge" },
+        { "root": "./src/scenarios/wpf" },
+        { "root": "./src/scenarios/wpfsfc" },
+        { "root": "./src/scenarios/xunittesttemplate" }
+    ]
+}

requirements.txt

@@ -1,7 +1,6 @@
 azure.storage.blob==12.13.0
 azure.storage.queue==12.4.0
 azure.identity==1.16.1
-dataclasses==0.8
 gitpython<=3.1.41
 urllib3==1.26.19
 opentelemetry-api==1.23.0

scripts/benchmarks_ci.py

@@ -21,14 +21,13 @@
 '''
 
 from argparse import ArgumentParser, ArgumentTypeError
-from datetime import datetime
 import json
 from logging import getLogger
 
 import os
 import shutil
 import sys
-from typing import Any, List, Optional
+from typing import Any, Optional
 
 from performance.common import get_repo_root_path, validate_supported_runtime, get_artifacts_directory, helixuploadroot
 from performance.logger import setup_loggers
@@ -44,11 +43,11 @@
 setup_tracing()
 tracer = get_tracer()
 
-@tracer.start_as_current_span(name="benchmarks_ci_init_tools") # type: ignore
+@tracer.start_as_current_span(name="benchmarks_ci_init_tools")
 def init_tools(
         architecture: str,
-        dotnet_versions: List[str],
-        target_framework_monikers: List[str],
+        dotnet_versions: list[str],
+        target_framework_monikers: list[str],
         verbose: bool,
         azure_feed_url: Optional[str] = None,
         internal_build_key: Optional[str] = None) -> None:
@@ -78,9 +77,6 @@ def init_tools(
 def add_arguments(parser: ArgumentParser) -> ArgumentParser:
     '''Adds new arguments to the specified ArgumentParser object.'''
 
-    if not isinstance(parser, ArgumentParser):
-        raise TypeError('Invalid parser.')
-
     # Download DotNet Cli
     dotnet.add_arguments(parser)
 
@@ -199,7 +195,7 @@ def __is_valid_dotnet_path(dp: str) -> str:
     return parser
 
 
-def __process_arguments(args: List[str]):
+def __process_arguments(args: list[str]):
     parser = ArgumentParser(
         description='Tool to run .NET micro benchmarks',
         allow_abbrev=False,
@@ -209,8 +205,8 @@ def __process_arguments(args: List[str]):
     add_arguments(parser)
     return parser.parse_args(args)
 
-@tracer.start_as_current_span("benchmarks_ci_main") # type: ignore
-def main(argv: List[str]):
+@tracer.start_as_current_span("benchmarks_ci_main")
+def main(argv: list[str]):
     validate_supported_runtime()
     args = __process_arguments(argv)
     verbose = not args.quiet
@@ -223,9 +219,7 @@ def main(argv: List[str]):
     if not args.frameworks:
         raise Exception("Framework version (-f) must be specified.")
 
-    target_framework_monikers = dotnet \
-        .FrameworkAction \
-        .get_target_framework_monikers(args.frameworks)
+    target_framework_monikers = dotnet.get_target_framework_monikers(args.frameworks)
     # Acquire necessary tools (dotnet)
     if not args.dotnet_path:
         init_tools(
@@ -303,7 +297,7 @@ def main(argv: List[str]):
                 # Create a combined JSON file that contains all the reports
                 combined_file_prefix = "" if args.partition is None else f"Partition{args.partition}-"
                 with open(os.path.join(helix_upload_root, f"{combined_file_prefix}combined-perf-lab-report.json"), "w", encoding="utf8") as all_reports_file:
-                    all_reports: List[Any] = []
+                    all_reports: list[Any] = []
                     for file in glob(reports_globpath, recursive=True):
                         with open(file, 'r', encoding="utf8") as report_file:
                             try:

scripts/benchmarks_local.py

@@ -3,7 +3,7 @@
 import platform
 import shutil
 import sys
-from typing import List, Optional
+from typing import Optional
 import xml.etree.ElementTree as xmlTree
 from argparse import ArgumentParser, ArgumentTypeError, Namespace
 from datetime import datetime
@@ -53,7 +53,7 @@ def is_running_as_admin(parsed_args: Namespace) -> bool:
     if is_windows(parsed_args):
         import ctypes
         return ctypes.windll.shell32.IsUserAnAdmin()
-    return os.getuid() == 0 # type: ignore We know that os.getuid() is a method on Unix-like systems, ignore the pylance unknown type error for getuid.
+    return os.getuid() == 0
 
 def kill_dotnet_processes(parsed_args: Namespace):
     if not parsed_args.kill_dotnet_processes:
@@ -72,10 +72,10 @@ def enum_name_to_enum(enum_type: EnumMeta, enum_name: str):
     except KeyError as exc:
         raise ArgumentTypeError(f"Invalid run type name {enum_name}.") from exc
 
-def enum_name_list_to_enum_list(enum_type: EnumMeta, enum_name_list: List[str]):
+def enum_name_list_to_enum_list(enum_type: EnumMeta, enum_name_list: list[str]):
     return [enum_name_to_enum(enum_type, enum_name) for enum_name in enum_name_list]
 
-def check_for_runtype_specified(parsed_args: Namespace, run_types_to_check: List[RunType]) -> bool:
+def check_for_runtype_specified(parsed_args: Namespace, run_types_to_check: list[RunType]) -> bool:
     for run_type in run_types_to_check:
         if run_type.name in parsed_args.run_type_names:
             return True
@@ -93,7 +93,7 @@ def copy_directory_contents(src_dir: str, dest_dir: str):
             shutil.copy2(os.path.join(src_dirpath, src_filename), dest_dirpath)
 
 # Builds libs and corerun by default
-def build_runtime_dependency(parsed_args: Namespace, repo_path: str, subset: str = "clr+libs", configuration: str = "Release", os_override = "", arch_override = "", additional_args: Optional[List[str]] = None):
+def build_runtime_dependency(parsed_args: Namespace, repo_path: str, subset: str = "clr+libs", configuration: str = "Release", os_override: str = "", arch_override: str = "", additional_args: Optional[list[str]] = None):
     if additional_args is None:
         additional_args = []
 
@@ -117,13 +117,13 @@ def build_runtime_dependency(parsed_args: Namespace, repo_path: str, subset: str
             ] + additional_args
     RunCommand(build_libs_and_corerun_command, verbose=True).run(os.path.join(repo_path, "eng"))
 
-def run_runtime_dotnet(repo_path: str, args: Optional[List[str]] = None):
+def run_runtime_dotnet(repo_path: str, args: Optional[list[str]] = None):
     if args is None:
         args = []
     dotnet_command = ["./dotnet.sh"] + args
     RunCommand(dotnet_command, verbose=True).run(repo_path)
 
-def generate_layout(parsed_args: Namespace, repo_path: str, additional_args: Optional[List[str]] = None):
+def generate_layout(parsed_args: Namespace, repo_path: str, additional_args: Optional[list[str]] = None):
     if additional_args is None:
         additional_args = []
 
@@ -264,10 +264,10 @@ def generate_all_runtype_dependencies(parsed_args: Namespace, repo_path: str, co
 
         getLogger().info("Finished generating dependencies for %s run types in %s and stored in %s.", ' '.join(map(str, parsed_args.run_type_names)), repo_path, parsed_args.artifact_storage_path)
 
-def generate_combined_benchmark_ci_args(parsed_args: Namespace, specific_run_type: RunType, all_commits: List[str]) -> List[str]:
+def generate_combined_benchmark_ci_args(parsed_args: Namespace, specific_run_type: RunType, all_commits: list[str]) -> list[str]:
     getLogger().info("Generating benchmark_ci.py arguments for %s run type using artifacts in %s.", specific_run_type.name, parsed_args.artifact_storage_path)
     bdn_args_unescaped: list[str] = []
-    benchmark_ci_args = [
+    benchmark_ci_args: list[str] = [
         '--architecture', parsed_args.architecture,
         '--frameworks', parsed_args.framework,
         '--dotnet-path', parsed_args.dotnet_dir_path,
@@ -330,10 +330,10 @@ def generate_combined_benchmark_ci_args(parsed_args: Namespace, specific_run_typ
     getLogger().info("Finished generating benchmark_ci.py arguments for %s run type using artifacts in %s.", specific_run_type.name, parsed_args.artifact_storage_path)
     return benchmark_ci_args
 
-def generate_single_benchmark_ci_args(parsed_args: Namespace, specific_run_type: RunType, commit: str) -> List[str]:
+def generate_single_benchmark_ci_args(parsed_args: Namespace, specific_run_type: RunType, commit: str) -> list[str]:
     getLogger().info("Generating benchmark_ci.py arguments for %s run type using artifacts in %s.", specific_run_type.name, parsed_args.artifact_storage_path)
     bdn_args_unescaped: list[str] = []
-    benchmark_ci_args = [
+    benchmark_ci_args: list[str] = [
         '--architecture', parsed_args.architecture,
         '--frameworks', parsed_args.framework,
         '--csproj', parsed_args.csproj,
@@ -445,7 +445,7 @@ def generate_artifacts_for_commit(parsed_args: Namespace, repo_url: str, repo_di
         getLogger().info("Running for %s at %s.", repo_path, commit)
 
         if not os.path.exists(repo_path):
-            repo = Repo.clone_from(repo_url, repo_path) # type: ignore 'Type of "clone_from" is partially unknown', we know it is a method and returns a Repo
+            repo = Repo.clone_from(repo_url, repo_path)
             repo.git.checkout(commit, '-f')
             repo.git.show('HEAD')
         else:
@@ -458,7 +458,7 @@ def generate_artifacts_for_commit(parsed_args: Namespace, repo_url: str, repo_di
     generate_all_runtype_dependencies(parsed_args, repo_path, commit, (is_local and not parsed_args.skip_local_rebuild) or parsed_args.rebuild_artifacts)
 
 # Run tests on the local machine
-def run_benchmarks(parsed_args: Namespace, commits: List[str]) -> None:
+def run_benchmarks(parsed_args: Namespace, commits: list[str]) -> None:
     # Generate the correct benchmarks_ci.py arguments for the run type
     for run_type_meta in enum_name_list_to_enum_list(RunType, parsed_args.run_type_names):
         # Run the benchmarks_ci.py test and save results
@@ -505,7 +505,7 @@ def check_references_exist_and_add_branch_commits(repo_url: str, references: lis
     repo_combined_path = os.path.join(repo_storage_path, repo_dir)
     if not os.path.exists(repo_combined_path):
         getLogger().debug("Cloning %s to %s.", repo_url, repo_combined_path)
-        repo = Repo.clone_from(repo_url, repo_combined_path) # type: ignore 'Type of "clone_from" is partially unknown', we know it is a method and returns a Repo
+        repo = Repo.clone_from(repo_url, repo_combined_path)
     else:
         repo = Repo(repo_combined_path)
         repo.remotes.origin.fetch()
@@ -564,11 +564,12 @@ def get_default_os():
     else:
         raise NotImplementedError(f"Unsupported operating system: {system}.")
 
-def __main(args: List[str]):
+def __main(args: list[str]):
     # Define the ArgumentParser
     parser = ArgumentParser(description='Run local benchmarks for the Performance repo.', conflict_handler='resolve')
     add_arguments(parser)
     parsed_args = parser.parse_args(args)
+    assert isinstance(parsed_args.artifact_storage_path, str)
     parsed_args.dotnet_dir_path = os.path.join(parsed_args.artifact_storage_path, "dotnet")
 
     setup_loggers(verbose=parsed_args.verbose)
@@ -587,9 +588,9 @@ def __main(args: List[str]):
 
     # If list cached builds is specified, list the cached builds and exit
     if parsed_args.list_cached_builds:
-        for folder in os.listdir(parsed_args.artifact_storage_path): # type: ignore warning about folder type being unknown, we know it is a string
+        for folder in os.listdir(parsed_args.artifact_storage_path):
             if any(run_type.name in folder for run_type in RunType):
-                getLogger().info(folder) # type: ignore We know folder is a string
+                getLogger().info(folder)
         return
 
     # Check to make sure we have something specified to test

scripts/benchmarks_monthly.py

@@ -6,7 +6,6 @@
 monthly manual performance runs.
 '''
 
-from typing import Dict, List
 from performance.common import get_machine_architecture
 from performance.logger import setup_loggers
 from argparse import ArgumentParser
@@ -36,7 +35,7 @@
     'net6.0': { 'tfm': 'net6.0' }
 }
 
-def get_version_from_name(name: str) -> Dict[str, str]:
+def get_version_from_name(name: str) -> dict[str, str]:
     if name in VERSIONS:
         return VERSIONS[name]
 
@@ -45,9 +44,6 @@ def get_version_from_name(name: str) -> Dict[str, str]:
 def add_arguments(parser: ArgumentParser) -> ArgumentParser:
     # Adds new arguments to the specified ArgumentParser object.
 
-    if not isinstance(parser, ArgumentParser):
-        raise TypeError('Invalid parser.')
-
     parser.add_argument(
         'versions',
         nargs='+',
@@ -114,7 +110,7 @@ def add_arguments(parser: ArgumentParser) -> ArgumentParser:
 
     return parser
 
-def __process_arguments(args: List[str]):
+def __process_arguments(args: list[str]):
     parser = ArgumentParser(
         description='Tool to execute the monthly manual micro benchmark performance runs',
         allow_abbrev=False
@@ -123,7 +119,7 @@ def __process_arguments(args: List[str]):
     add_arguments(parser)
     return parser.parse_args(args)
 
-def __main(argv: List[str]):
+def __main(argv: list[str]):
     setup_loggers(verbose=True)
 
     args = __process_arguments(argv)