From d928e0a68e319440d4a83e2994eeb05a0fb866f4 Mon Sep 17 00:00:00 2001 From: Jim Fitzpatrick Date: Wed, 1 Oct 2025 15:19:06 +0100 Subject: [PATCH] BUG: Equality Vs Identity Fix a number of instances where Equality was being checked over Identity. While no bug currently existed, this pattern was could have caused an extremely hard to track down bug. ```python >>> 1 == True # Equality True >>> 1 is True # Identity False ``` Signed-off-by: Jim Fitzpatrick --- .../common/kubernetes_cluster/auth.py | 8 ++++---- .../common/kubernetes_cluster/test_auth.py | 16 ++++++++-------- src/codeflare_sdk/common/kueue/kueue.py | 2 +- src/codeflare_sdk/common/kueue/test_kueue.py | 2 +- .../common/utils/test_generate_cert.py | 8 ++++---- src/codeflare_sdk/ray/appwrapper/test_awload.py | 12 ++++++------ src/codeflare_sdk/ray/appwrapper/test_status.py | 14 +++++++------- .../ray/cluster/build_ray_cluster.py | 2 +- src/codeflare_sdk/ray/cluster/cluster.py | 6 +++--- src/codeflare_sdk/ray/cluster/test_cluster.py | 4 ++-- src/codeflare_sdk/ray/cluster/test_config.py | 8 ++++---- src/codeflare_sdk/ray/cluster/test_status.py | 12 ++++++------ tests/e2e/mnist.py | 6 +----- tests/e2e/support.py | 12 ++---------- 14 files changed, 50 insertions(+), 62 deletions(-) diff --git a/src/codeflare_sdk/common/kubernetes_cluster/auth.py b/src/codeflare_sdk/common/kubernetes_cluster/auth.py index db105afc..b71ff9e1 100644 --- a/src/codeflare_sdk/common/kubernetes_cluster/auth.py +++ b/src/codeflare_sdk/common/kubernetes_cluster/auth.py @@ -151,7 +151,7 @@ def load_kube_config(self): global config_path global api_client try: - if self.kube_config_path == None: + if self.kube_config_path is None: return "Please specify a config file path" config_path = self.kube_config_path api_client = None @@ -183,7 +183,7 @@ def config_check() -> str: global config_path global api_client home_directory = os.path.expanduser("~") - if config_path == None and api_client == None: + if config_path is None and api_client is None: if os.path.isfile("%s/.kube/config" % home_directory): try: config.load_kube_config() @@ -199,7 +199,7 @@ def config_check() -> str: "Action not permitted, have you put in correct/up-to-date auth credentials?" ) - if config_path != None and api_client == None: + if config_path is not None and api_client is None: return config_path @@ -237,7 +237,7 @@ def get_api_client() -> client.ApiClient: client.ApiClient: The Kubernetes API client object. """ - if api_client != None: + if api_client is not None: return api_client to_return = client.ApiClient() _client_with_cert(to_return) diff --git a/src/codeflare_sdk/common/kubernetes_cluster/test_auth.py b/src/codeflare_sdk/common/kubernetes_cluster/test_auth.py index be9e90f5..d37afb85 100644 --- a/src/codeflare_sdk/common/kubernetes_cluster/test_auth.py +++ b/src/codeflare_sdk/common/kubernetes_cluster/test_auth.py @@ -30,20 +30,20 @@ def test_token_auth_creation(): token_auth = TokenAuthentication(token="token", server="server") assert token_auth.token == "token" assert token_auth.server == "server" - assert token_auth.skip_tls == False - assert token_auth.ca_cert_path == None + assert token_auth.skip_tls is False + assert token_auth.ca_cert_path is None token_auth = TokenAuthentication(token="token", server="server", skip_tls=True) assert token_auth.token == "token" assert token_auth.server == "server" - assert token_auth.skip_tls == True - assert token_auth.ca_cert_path == None + assert token_auth.skip_tls is True + assert token_auth.ca_cert_path is None os.environ["CF_SDK_CA_CERT_PATH"] = "/etc/pki/tls/custom-certs/ca-bundle.crt" token_auth = TokenAuthentication(token="token", server="server", skip_tls=False) assert token_auth.token == "token" assert token_auth.server == "server" - assert token_auth.skip_tls == False + assert token_auth.skip_tls is False assert token_auth.ca_cert_path == "/etc/pki/tls/custom-certs/ca-bundle.crt" os.environ.pop("CF_SDK_CA_CERT_PATH") @@ -55,7 +55,7 @@ def test_token_auth_creation(): ) assert token_auth.token == "token" assert token_auth.server == "server" - assert token_auth.skip_tls == False + assert token_auth.skip_tls is False assert token_auth.ca_cert_path == f"{parent}/tests/auth-test.crt" @@ -116,7 +116,7 @@ def test_config_check_with_incluster_config(mocker): mocker.patch("codeflare_sdk.common.kubernetes_cluster.auth.api_client", None) result = config_check() - assert result == None + assert result is None def test_config_check_with_existing_config_file(mocker): @@ -127,7 +127,7 @@ def test_config_check_with_existing_config_file(mocker): mocker.patch("codeflare_sdk.common.kubernetes_cluster.auth.api_client", None) result = config_check() - assert result == None + assert result is None def test_config_check_with_config_path_and_no_api_client(mocker): diff --git a/src/codeflare_sdk/common/kueue/kueue.py b/src/codeflare_sdk/common/kueue/kueue.py index 00f3364a..a13792a4 100644 --- a/src/codeflare_sdk/common/kueue/kueue.py +++ b/src/codeflare_sdk/common/kueue/kueue.py @@ -163,7 +163,7 @@ def add_queue_label(item: dict, namespace: str, local_queue: Optional[str]): If the provided or default local queue does not exist in the namespace. """ lq_name = local_queue or get_default_kueue_name(namespace) - if lq_name == None: + if lq_name is None: return elif not local_queue_exists(namespace, lq_name): raise ValueError( diff --git a/src/codeflare_sdk/common/kueue/test_kueue.py b/src/codeflare_sdk/common/kueue/test_kueue.py index bbc54e9e..a6aabb45 100644 --- a/src/codeflare_sdk/common/kueue/test_kueue.py +++ b/src/codeflare_sdk/common/kueue/test_kueue.py @@ -36,7 +36,7 @@ def test_none_local_queue(mocker): config.local_queue = None cluster = Cluster(config) - assert cluster.config.local_queue == None + assert cluster.config.local_queue is None def test_cluster_creation_no_aw_local_queue(mocker): diff --git a/src/codeflare_sdk/common/utils/test_generate_cert.py b/src/codeflare_sdk/common/utils/test_generate_cert.py index b4439c20..b05120c0 100644 --- a/src/codeflare_sdk/common/utils/test_generate_cert.py +++ b/src/codeflare_sdk/common/utils/test_generate_cert.py @@ -43,10 +43,10 @@ def test_generate_ca_cert(): cert_pub_key_bytes = cert.public_key().public_bytes( Encoding.PEM, PublicFormat.SubjectPublicKeyInfo ) - assert type(key) == str - assert type(certificate) == str + assert isinstance(key, str) + assert isinstance(certificate, str) # Veirfy ca.cert is self signed - assert cert.verify_directly_issued_by(cert) == None + assert cert.verify_directly_issued_by(cert) is None # Verify cert has the public key bytes from the private key assert cert_pub_key_bytes == private_pub_key_bytes @@ -84,7 +84,7 @@ def test_generate_tls_cert(mocker): tls_cert = load_pem_x509_certificate(f.read().encode("utf-8")) with open(os.path.join("tls-cluster-namespace", "ca.crt"), "r") as f: root_cert = load_pem_x509_certificate(f.read().encode("utf-8")) - assert tls_cert.verify_directly_issued_by(root_cert) == None + assert tls_cert.verify_directly_issued_by(root_cert) is None def test_export_env(): diff --git a/src/codeflare_sdk/ray/appwrapper/test_awload.py b/src/codeflare_sdk/ray/appwrapper/test_awload.py index 3f45e1a5..1d59b27f 100644 --- a/src/codeflare_sdk/ray/appwrapper/test_awload.py +++ b/src/codeflare_sdk/ray/appwrapper/test_awload.py @@ -42,11 +42,11 @@ def test_AWManager_creation(mocker): testaw = AWManager(f"{aw_dir}test.yaml") assert testaw.name == "test" assert testaw.namespace == "ns" - assert testaw.submitted == False + assert testaw.submitted is False try: testaw = AWManager("fake") except Exception as e: - assert type(e) == FileNotFoundError + assert isinstance(e, FileNotFoundError) assert str(e) == "[Errno 2] No such file or directory: 'fake'" try: testaw = apply_template( @@ -56,7 +56,7 @@ def test_AWManager_creation(mocker): get_template_variables(), ) except Exception as e: - assert type(e) == ValueError + assert isinstance(e, ValueError) assert ( str(e) == f"{parent}/tests/test_cluster_yamls/appwrapper/test-case-bad.yaml is not a correctly formatted AppWrapper yaml" @@ -72,7 +72,7 @@ def test_AWManager_submit_remove(mocker, capsys): captured.out == "AppWrapper not submitted by this manager yet, nothing to remove\n" ) - assert testaw.submitted == False + assert testaw.submitted is False mocker.patch("kubernetes.config.load_kube_config", return_value="ignore") mocker.patch( "kubernetes.client.CustomObjectsApi.create_namespaced_custom_object", @@ -83,9 +83,9 @@ def test_AWManager_submit_remove(mocker, capsys): side_effect=arg_check_aw_del_effect, ) testaw.submit() - assert testaw.submitted == True + assert testaw.submitted is True testaw.remove() - assert testaw.submitted == False + assert testaw.submitted is False # Make sure to always keep this function last diff --git a/src/codeflare_sdk/ray/appwrapper/test_status.py b/src/codeflare_sdk/ray/appwrapper/test_status.py index a3fcf870..3c4be0fc 100644 --- a/src/codeflare_sdk/ray/appwrapper/test_status.py +++ b/src/codeflare_sdk/ray/appwrapper/test_status.py @@ -51,34 +51,34 @@ def test_cluster_status(mocker): ) status, ready = cf.status() assert status == CodeFlareClusterStatus.UNKNOWN - assert ready == False + assert ready is False mocker.patch( "codeflare_sdk.ray.cluster.cluster._app_wrapper_status", return_value=fake_aw ) status, ready = cf.status() assert status == CodeFlareClusterStatus.FAILED - assert ready == False + assert ready is False fake_aw.status = AppWrapperStatus.SUSPENDED status, ready = cf.status() assert status == CodeFlareClusterStatus.QUEUED - assert ready == False + assert ready is False fake_aw.status = AppWrapperStatus.RESUMING status, ready = cf.status() assert status == CodeFlareClusterStatus.STARTING - assert ready == False + assert ready is False fake_aw.status = AppWrapperStatus.RESETTING status, ready = cf.status() assert status == CodeFlareClusterStatus.STARTING - assert ready == False + assert ready is False fake_aw.status = AppWrapperStatus.RUNNING status, ready = cf.status() assert status == CodeFlareClusterStatus.UNKNOWN - assert ready == False + assert ready is False def aw_status_fields(group, version, namespace, plural, *args): @@ -97,7 +97,7 @@ def test_aw_status(mocker): side_effect=aw_status_fields, ) aw = _app_wrapper_status("test-aw", "test-ns") - assert aw == None + assert aw is None # Make sure to always keep this function last diff --git a/src/codeflare_sdk/ray/cluster/build_ray_cluster.py b/src/codeflare_sdk/ray/cluster/build_ray_cluster.py index e8b68919..e9d9b2d8 100644 --- a/src/codeflare_sdk/ray/cluster/build_ray_cluster.py +++ b/src/codeflare_sdk/ray/cluster/build_ray_cluster.py @@ -482,7 +482,7 @@ def add_queue_label(cluster: "codeflare_sdk.ray.cluster.Cluster", labels: dict): The add_queue_label() function updates the given base labels with the local queue label if Kueue exists on the Cluster """ lq_name = cluster.config.local_queue or get_default_local_queue(cluster, labels) - if lq_name == None: + if lq_name is None: return elif not local_queue_exists(cluster): # ValueError removed to pass validation to validating admission policy diff --git a/src/codeflare_sdk/ray/cluster/cluster.py b/src/codeflare_sdk/ray/cluster/cluster.py index 4eaa2000..1e5eb347 100644 --- a/src/codeflare_sdk/ray/cluster/cluster.py +++ b/src/codeflare_sdk/ray/cluster/cluster.py @@ -513,7 +513,7 @@ def cluster_dashboard_uri(self) -> str: ingress.metadata.name == f"ray-dashboard-{self.config.name}" or ingress.metadata.name.startswith(f"{self.config.name}-ingress") ): - if annotations == None: + if annotations is None: protocol = "http" elif "route.openshift.io/termination" in annotations: protocol = "https" @@ -865,7 +865,7 @@ def _check_aw_exists(name: str, namespace: str) -> bool: def _get_ingress_domain(self): # pragma: no cover config_check() - if self.config.namespace != None: + if self.config.namespace is not None: namespace = self.config.namespace else: namespace = get_current_namespace() @@ -1037,7 +1037,7 @@ def _map_to_ray_cluster(rc) -> Optional[RayCluster]: ingress.metadata.name == f"ray-dashboard-{rc['metadata']['name']}" or ingress.metadata.name.startswith(f"{rc['metadata']['name']}-ingress") ): - if annotations == None: + if annotations is None: protocol = "http" elif "route.openshift.io/termination" in annotations: protocol = "https" diff --git a/src/codeflare_sdk/ray/cluster/test_cluster.py b/src/codeflare_sdk/ray/cluster/test_cluster.py index ce684607..00528533 100644 --- a/src/codeflare_sdk/ray/cluster/test_cluster.py +++ b/src/codeflare_sdk/ray/cluster/test_cluster.py @@ -534,7 +534,7 @@ def test_wait_ready(mocker, capsys): cf.wait_ready(timeout=5) assert 1 == 0 except Exception as e: - assert type(e) == TimeoutError + assert isinstance(e, TimeoutError) captured = capsys.readouterr() assert ( @@ -606,7 +606,7 @@ def test_list_queue_rayclusters(mocker, capsys): ] mocker.patch("kubernetes.client.ApisApi", return_value=mock_api) - assert _is_openshift_cluster() == True + assert _is_openshift_cluster() is True mocker.patch( "kubernetes.client.CustomObjectsApi.list_namespaced_custom_object", return_value=get_obj_none("ray.io", "v1", "ns", "rayclusters"), diff --git a/src/codeflare_sdk/ray/cluster/test_config.py b/src/codeflare_sdk/ray/cluster/test_config.py index e405bc5b..f9896c71 100644 --- a/src/codeflare_sdk/ray/cluster/test_config.py +++ b/src/codeflare_sdk/ray/cluster/test_config.py @@ -85,7 +85,7 @@ def test_config_creation_all_parameters(mocker): assert cluster.config.num_workers == 10 assert cluster.config.worker_memory_requests == "12G" assert cluster.config.worker_memory_limits == "16G" - assert cluster.config.appwrapper == False + assert cluster.config.appwrapper is False assert cluster.config.envs == { "key1": "value1", "key2": "value2", @@ -93,14 +93,14 @@ def test_config_creation_all_parameters(mocker): } assert cluster.config.image == "example/ray:tag" assert cluster.config.image_pull_secrets == ["secret1", "secret2"] - assert cluster.config.write_to_file == True - assert cluster.config.verify_tls == True + assert cluster.config.write_to_file is True + assert cluster.config.verify_tls is True assert cluster.config.labels == {"key1": "value1", "key2": "value2"} assert cluster.config.worker_extended_resource_requests == {"nvidia.com/gpu": 1} assert ( cluster.config.extended_resource_mapping == expected_extended_resource_mapping ) - assert cluster.config.overwrite_default_resource_mapping == True + assert cluster.config.overwrite_default_resource_mapping is True assert cluster.config.local_queue == "local-queue-default" assert cluster.config.annotations == { "app.kubernetes.io/managed-by": "test-prefix", diff --git a/src/codeflare_sdk/ray/cluster/test_status.py b/src/codeflare_sdk/ray/cluster/test_status.py index 27eda49e..6cd2fd2e 100644 --- a/src/codeflare_sdk/ray/cluster/test_status.py +++ b/src/codeflare_sdk/ray/cluster/test_status.py @@ -67,7 +67,7 @@ def test_cluster_status(mocker): ) status, ready = cf.status() assert status == CodeFlareClusterStatus.UNKNOWN - assert ready == False + assert ready is False mocker.patch( "codeflare_sdk.ray.cluster.cluster._ray_cluster_status", return_value=fake_ray @@ -75,22 +75,22 @@ def test_cluster_status(mocker): status, ready = cf.status() assert status == CodeFlareClusterStatus.STARTING - assert ready == False + assert ready is False fake_ray.status = RayClusterStatus.FAILED status, ready = cf.status() assert status == CodeFlareClusterStatus.FAILED - assert ready == False + assert ready is False fake_ray.status = RayClusterStatus.UNHEALTHY status, ready = cf.status() assert status == CodeFlareClusterStatus.FAILED - assert ready == False + assert ready is False fake_ray.status = RayClusterStatus.READY status, ready = cf.status() assert status == CodeFlareClusterStatus.READY - assert ready == True + assert ready is True def rc_status_fields(group, version, namespace, plural, *args): @@ -109,7 +109,7 @@ def test_rc_status(mocker): side_effect=rc_status_fields, ) rc = _ray_cluster_status("test-rc", "test-ns") - assert rc == None + assert rc is None # Make sure to always keep this function last diff --git a/tests/e2e/mnist.py b/tests/e2e/mnist.py index 143a6b6c..7505092c 100644 --- a/tests/e2e/mnist.py +++ b/tests/e2e/mnist.py @@ -141,11 +141,7 @@ def prepare_data(self): # download print("Downloading MNIST dataset...") - if ( - STORAGE_BUCKET_EXISTS - and os.environ.get("AWS_DEFAULT_ENDPOINT") != "" - and os.environ.get("AWS_DEFAULT_ENDPOINT") != None - ): + if STORAGE_BUCKET_EXISTS and os.environ.get("AWS_DEFAULT_ENDPOINT", "") != "": print("Using storage bucket to download datasets...") dataset_dir = os.path.join(self.data_dir, "MNIST/raw") diff --git a/tests/e2e/support.py b/tests/e2e/support.py index fe9261a2..5ce95041 100644 --- a/tests/e2e/support.py +++ b/tests/e2e/support.py @@ -42,11 +42,7 @@ def get_setup_env_variables(**kwargs): env_vars[str(key)] = value # Use specified pip index url instead of default(https://pypi.org/simple) if related environment variables exists - if ( - "PIP_INDEX_URL" in os.environ - and os.environ.get("PIP_INDEX_URL") != None - and os.environ.get("PIP_INDEX_URL") != "" - ): + if os.environ.get("PIP_INDEX_URL", "") != "": env_vars["PIP_INDEX_URL"] = os.environ.get("PIP_INDEX_URL") env_vars["PIP_TRUSTED_HOST"] = os.environ.get("PIP_TRUSTED_HOST") else: @@ -54,11 +50,7 @@ def get_setup_env_variables(**kwargs): env_vars["PIP_TRUSTED_HOST"] = "pypi.org" # Use specified storage bucket reference from which to download datasets - if ( - "AWS_DEFAULT_ENDPOINT" in os.environ - and os.environ.get("AWS_DEFAULT_ENDPOINT") != None - and os.environ.get("AWS_DEFAULT_ENDPOINT") != "" - ): + if os.environ.get("AWS_DEFAULT_ENDPOINT", "") != "": env_vars["AWS_DEFAULT_ENDPOINT"] = os.environ.get("AWS_DEFAULT_ENDPOINT") env_vars["AWS_ACCESS_KEY_ID"] = os.environ.get("AWS_ACCESS_KEY_ID") env_vars["AWS_SECRET_ACCESS_KEY"] = os.environ.get("AWS_SECRET_ACCESS_KEY")