Skip to content

Commit 51e2b12

Browse files
committed
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 <jfitzpat@redhat.com>
1 parent 67df4f5 commit 51e2b12

File tree

16 files changed

+83
-113
lines changed

16 files changed

+83
-113
lines changed

src/codeflare_sdk/common/kubernetes_cluster/auth.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ def load_kube_config(self):
151151
global config_path
152152
global api_client
153153
try:
154-
if self.kube_config_path == None:
154+
if self.kube_config_path is None:
155155
return "Please specify a config file path"
156156
config_path = self.kube_config_path
157157
api_client = None
@@ -183,7 +183,7 @@ def config_check() -> str:
183183
global config_path
184184
global api_client
185185
home_directory = os.path.expanduser("~")
186-
if config_path == None and api_client == None:
186+
if config_path is None and api_client is None:
187187
if os.path.isfile("%s/.kube/config" % home_directory):
188188
try:
189189
config.load_kube_config()
@@ -199,7 +199,7 @@ def config_check() -> str:
199199
"Action not permitted, have you put in correct/up-to-date auth credentials?"
200200
)
201201

202-
if config_path != None and api_client == None:
202+
if config_path is not None and api_client is None:
203203
return config_path
204204

205205

@@ -237,7 +237,7 @@ def get_api_client() -> client.ApiClient:
237237
client.ApiClient:
238238
The Kubernetes API client object.
239239
"""
240-
if api_client != None:
240+
if api_client is not None:
241241
return api_client
242242
to_return = client.ApiClient()
243243
_client_with_cert(to_return)

src/codeflare_sdk/common/kubernetes_cluster/test_auth.py

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -30,20 +30,20 @@ def test_token_auth_creation():
3030
token_auth = TokenAuthentication(token="token", server="server")
3131
assert token_auth.token == "token"
3232
assert token_auth.server == "server"
33-
assert token_auth.skip_tls == False
34-
assert token_auth.ca_cert_path == None
33+
assert token_auth.skip_tls is False
34+
assert token_auth.ca_cert_path is None
3535

3636
token_auth = TokenAuthentication(token="token", server="server", skip_tls=True)
3737
assert token_auth.token == "token"
3838
assert token_auth.server == "server"
39-
assert token_auth.skip_tls == True
40-
assert token_auth.ca_cert_path == None
39+
assert token_auth.skip_tls is True
40+
assert token_auth.ca_cert_path is None
4141

4242
os.environ["CF_SDK_CA_CERT_PATH"] = "/etc/pki/tls/custom-certs/ca-bundle.crt"
4343
token_auth = TokenAuthentication(token="token", server="server", skip_tls=False)
4444
assert token_auth.token == "token"
4545
assert token_auth.server == "server"
46-
assert token_auth.skip_tls == False
46+
assert token_auth.skip_tls is False
4747
assert token_auth.ca_cert_path == "/etc/pki/tls/custom-certs/ca-bundle.crt"
4848
os.environ.pop("CF_SDK_CA_CERT_PATH")
4949

@@ -55,7 +55,7 @@ def test_token_auth_creation():
5555
)
5656
assert token_auth.token == "token"
5757
assert token_auth.server == "server"
58-
assert token_auth.skip_tls == False
58+
assert token_auth.skip_tls is False
5959
assert token_auth.ca_cert_path == f"{parent}/tests/auth-test.crt"
6060

6161

@@ -116,7 +116,7 @@ def test_config_check_with_incluster_config(mocker):
116116
mocker.patch("codeflare_sdk.common.kubernetes_cluster.auth.api_client", None)
117117

118118
result = config_check()
119-
assert result == None
119+
assert result is None
120120

121121

122122
def test_config_check_with_existing_config_file(mocker):
@@ -127,7 +127,7 @@ def test_config_check_with_existing_config_file(mocker):
127127
mocker.patch("codeflare_sdk.common.kubernetes_cluster.auth.api_client", None)
128128

129129
result = config_check()
130-
assert result == None
130+
assert result is None
131131

132132

133133
def test_config_check_with_config_path_and_no_api_client(mocker):

src/codeflare_sdk/common/kueue/kueue.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@ def add_queue_label(item: dict, namespace: str, local_queue: Optional[str]):
164164
If the provided or default local queue does not exist in the namespace.
165165
"""
166166
lq_name = local_queue or get_default_kueue_name(namespace)
167-
if lq_name == None:
167+
if lq_name is None:
168168
return
169169
elif not local_queue_exists(namespace, lq_name):
170170
raise ValueError(

src/codeflare_sdk/common/kueue/test_kueue.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ def test_none_local_queue(mocker):
3636
config.local_queue = None
3737

3838
cluster = Cluster(config)
39-
assert cluster.config.local_queue == None
39+
assert cluster.config.local_queue is None
4040

4141

4242
def test_cluster_creation_no_aw_local_queue(mocker):

src/codeflare_sdk/common/utils/test_generate_cert.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -43,10 +43,10 @@ def test_generate_ca_cert():
4343
cert_pub_key_bytes = cert.public_key().public_bytes(
4444
Encoding.PEM, PublicFormat.SubjectPublicKeyInfo
4545
)
46-
assert type(key) == str
47-
assert type(certificate) == str
46+
assert isinstance(key, str)
47+
assert isinstance(certificate, str)
4848
# Veirfy ca.cert is self signed
49-
assert cert.verify_directly_issued_by(cert) == None
49+
assert cert.verify_directly_issued_by(cert) is None
5050
# Verify cert has the public key bytes from the private key
5151
assert cert_pub_key_bytes == private_pub_key_bytes
5252

@@ -84,7 +84,7 @@ def test_generate_tls_cert(mocker):
8484
tls_cert = load_pem_x509_certificate(f.read().encode("utf-8"))
8585
with open(os.path.join("tls-cluster-namespace", "ca.crt"), "r") as f:
8686
root_cert = load_pem_x509_certificate(f.read().encode("utf-8"))
87-
assert tls_cert.verify_directly_issued_by(root_cert) == None
87+
assert tls_cert.verify_directly_issued_by(root_cert) is None
8888

8989

9090
def test_export_env():

src/codeflare_sdk/ray/appwrapper/test_awload.py

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -42,11 +42,11 @@ def test_AWManager_creation(mocker):
4242
testaw = AWManager(f"{aw_dir}test.yaml")
4343
assert testaw.name == "test"
4444
assert testaw.namespace == "ns"
45-
assert testaw.submitted == False
45+
assert testaw.submitted is False
4646
try:
4747
testaw = AWManager("fake")
4848
except Exception as e:
49-
assert type(e) == FileNotFoundError
49+
assert isinstance(e, FileNotFoundError)
5050
assert str(e) == "[Errno 2] No such file or directory: 'fake'"
5151
try:
5252
testaw = apply_template(
@@ -56,7 +56,7 @@ def test_AWManager_creation(mocker):
5656
get_template_variables(),
5757
)
5858
except Exception as e:
59-
assert type(e) == ValueError
59+
assert isinstance(e, ValueError)
6060
assert (
6161
str(e)
6262
== 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):
7272
captured.out
7373
== "AppWrapper not submitted by this manager yet, nothing to remove\n"
7474
)
75-
assert testaw.submitted == False
75+
assert testaw.submitted is False
7676
mocker.patch("kubernetes.config.load_kube_config", return_value="ignore")
7777
mocker.patch(
7878
"kubernetes.client.CustomObjectsApi.create_namespaced_custom_object",
@@ -83,9 +83,9 @@ def test_AWManager_submit_remove(mocker, capsys):
8383
side_effect=arg_check_aw_del_effect,
8484
)
8585
testaw.submit()
86-
assert testaw.submitted == True
86+
assert testaw.submitted is True
8787
testaw.remove()
88-
assert testaw.submitted == False
88+
assert testaw.submitted is False
8989

9090

9191
# Make sure to always keep this function last

src/codeflare_sdk/ray/appwrapper/test_status.py

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -51,34 +51,34 @@ def test_cluster_status(mocker):
5151
)
5252
status, ready = cf.status()
5353
assert status == CodeFlareClusterStatus.UNKNOWN
54-
assert ready == False
54+
assert ready is False
5555

5656
mocker.patch(
5757
"codeflare_sdk.ray.cluster.cluster._app_wrapper_status", return_value=fake_aw
5858
)
5959
status, ready = cf.status()
6060
assert status == CodeFlareClusterStatus.FAILED
61-
assert ready == False
61+
assert ready is False
6262

6363
fake_aw.status = AppWrapperStatus.SUSPENDED
6464
status, ready = cf.status()
6565
assert status == CodeFlareClusterStatus.QUEUED
66-
assert ready == False
66+
assert ready is False
6767

6868
fake_aw.status = AppWrapperStatus.RESUMING
6969
status, ready = cf.status()
7070
assert status == CodeFlareClusterStatus.STARTING
71-
assert ready == False
71+
assert ready is False
7272

7373
fake_aw.status = AppWrapperStatus.RESETTING
7474
status, ready = cf.status()
7575
assert status == CodeFlareClusterStatus.STARTING
76-
assert ready == False
76+
assert ready is False
7777

7878
fake_aw.status = AppWrapperStatus.RUNNING
7979
status, ready = cf.status()
8080
assert status == CodeFlareClusterStatus.UNKNOWN
81-
assert ready == False
81+
assert ready is False
8282

8383

8484
def aw_status_fields(group, version, namespace, plural, *args):
@@ -97,7 +97,7 @@ def test_aw_status(mocker):
9797
side_effect=aw_status_fields,
9898
)
9999
aw = _app_wrapper_status("test-aw", "test-ns")
100-
assert aw == None
100+
assert aw is None
101101

102102

103103
# Make sure to always keep this function last

src/codeflare_sdk/ray/cluster/build_ray_cluster.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -469,7 +469,7 @@ def add_queue_label(cluster: "codeflare_sdk.ray.cluster.Cluster", labels: dict):
469469
The add_queue_label() function updates the given base labels with the local queue label if Kueue exists on the Cluster
470470
"""
471471
lq_name = cluster.config.local_queue or get_default_local_queue(cluster, labels)
472-
if lq_name == None:
472+
if lq_name is None:
473473
return
474474
elif not local_queue_exists(cluster):
475475
# ValueError removed to pass validation to validating admission policy

src/codeflare_sdk/ray/cluster/cluster.py

Lines changed: 20 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -18,51 +18,33 @@
1818
cluster setup queue, a list of all existing clusters, and the user's working namespace.
1919
"""
2020

21-
from time import sleep
22-
from typing import List, Optional, Tuple, Dict
2321
import copy
24-
25-
from ray.job_submission import JobSubmissionClient, JobStatus
22+
import os
2623
import time
2724
import uuid
2825
import warnings
26+
from time import sleep
27+
from typing import Dict, List, Optional, Tuple
2928

30-
from ...common.utils import get_current_namespace
29+
import requests
30+
import yaml
31+
from kubernetes import client
32+
from kubernetes import client as k8s_client
33+
from kubernetes import config
34+
from kubernetes.client.rest import ApiException
35+
from kubernetes.dynamic import DynamicClient
36+
from ray.job_submission import JobStatus, JobSubmissionClient
3137

32-
from ...common.kubernetes_cluster.auth import (
33-
config_check,
34-
get_api_client,
35-
)
38+
from ...common import _kube_api_error_handling
39+
from ...common.kubernetes_cluster.auth import config_check, get_api_client
40+
from ...common.utils import get_current_namespace
41+
from ...common.widgets.widgets import cluster_apply_down_buttons, is_notebook
42+
from ..appwrapper import AppWrapper, AppWrapperStatus
3643
from . import pretty_print
3744
from .build_ray_cluster import build_ray_cluster, head_worker_gpu_count_from_cluster
3845
from .build_ray_cluster import write_to_file as write_cluster_to_file
39-
from ...common import _kube_api_error_handling
40-
4146
from .config import ClusterConfiguration
42-
from .status import (
43-
CodeFlareClusterStatus,
44-
RayCluster,
45-
RayClusterStatus,
46-
)
47-
from ..appwrapper import (
48-
AppWrapper,
49-
AppWrapperStatus,
50-
)
51-
from ...common.widgets.widgets import (
52-
cluster_apply_down_buttons,
53-
is_notebook,
54-
)
55-
from kubernetes import client
56-
import yaml
57-
import os
58-
import requests
59-
60-
from kubernetes import config
61-
from kubernetes.dynamic import DynamicClient
62-
from kubernetes import client as k8s_client
63-
from kubernetes.client.rest import ApiException
64-
65-
from kubernetes.client.rest import ApiException
47+
from .status import CodeFlareClusterStatus, RayCluster, RayClusterStatus
6648

6749
CF_SDK_FIELD_MANAGER = "codeflare-sdk"
6850

@@ -546,7 +528,7 @@ def cluster_dashboard_uri(self) -> str:
546528
ingress.metadata.name == f"ray-dashboard-{self.config.name}"
547529
or ingress.metadata.name.startswith(f"{self.config.name}-ingress")
548530
):
549-
if annotations == None:
531+
if annotations is None:
550532
protocol = "http"
551533
elif "route.openshift.io/termination" in annotations:
552534
protocol = "https"
@@ -874,7 +856,7 @@ def _check_aw_exists(name: str, namespace: str) -> bool:
874856
def _get_ingress_domain(self): # pragma: no cover
875857
config_check()
876858

877-
if self.config.namespace != None:
859+
if self.config.namespace is not None:
878860
namespace = self.config.namespace
879861
else:
880862
namespace = get_current_namespace()
@@ -1052,7 +1034,7 @@ def _map_to_ray_cluster(rc) -> Optional[RayCluster]:
10521034
ingress.metadata.name == f"ray-dashboard-{rc_name}"
10531035
or ingress.metadata.name.startswith(f"{rc_name}-ingress")
10541036
):
1055-
if annotations == None:
1037+
if annotations is None:
10561038
protocol = "http"
10571039
elif "route.openshift.io/termination" in annotations:
10581040
protocol = "https"

src/codeflare_sdk/ray/cluster/test_cluster.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -537,7 +537,7 @@ def test_wait_ready(mocker, capsys):
537537
cf.wait_ready(timeout=5)
538538
assert 1 == 0
539539
except Exception as e:
540-
assert type(e) == TimeoutError
540+
assert isinstance(e, TimeoutError)
541541

542542
captured = capsys.readouterr()
543543
assert (
@@ -613,7 +613,7 @@ def test_list_queue_rayclusters(mocker, capsys):
613613
]
614614
mocker.patch("kubernetes.client.ApisApi", return_value=mock_api)
615615

616-
assert _is_openshift_cluster() == True
616+
assert _is_openshift_cluster() is True
617617
mocker.patch(
618618
"kubernetes.client.CustomObjectsApi.list_namespaced_custom_object",
619619
return_value=get_obj_none("ray.io", "v1", "ns", "rayclusters"),

0 commit comments

Comments
 (0)