Move env var checks into patch_needed() function #22

gmarkall · 2022-08-15T09:12:42Z

With this change, the patch_needed() function can also be used from other libraries without them needing to reimplement the logic and env vars to decide whether to check the driver and runtime versions.

I also updated the message about not patching the Numba codegen - the reason for not patching the codegen is not always the driver version (sometimes it's the environment variable) and the reason should be apparent from previous log lines, so it now only states that the codegen is not being patched.

Some of my experiments/checks with this change (similar to those I did for #20):

$ PTXCOMPILER_APPLY_NUMBA_CODEGEN_PATCH=1 NUMBA_CUDA_LOG_LEVEL=DEBUG python -c "from ptxcompiler.patch import patch_numba_codegen_if_needed; patch_numba_codegen_if_needed()"
== CUDA (ptxcompiler) [2612] DEBUG -- PTXCOMPILER_APPLY_NUMBA_CODEGEN_PATCH=1
== CUDA (ptxcompiler) [2612] DEBUG -- Patching Numba codegen for forward compatibility

$ PTXCOMPILER_APPLY_NUMBA_CODEGEN_PATCH=0 NUMBA_CUDA_LOG_LEVEL=DEBUG python -c "from ptxcompiler.patch import patch_numba_codegen_if_needed; patch_numba_codegen_if_needed()"
== CUDA (ptxcompiler) [297] DEBUG -- PTXCOMPILER_APPLY_NUMBA_CODEGEN_PATCH=0
== CUDA (ptxcompiler) [709] DEBUG -- CUDA Driver version 11.7
== CUDA (ptxcompiler) [709] DEBUG -- CUDA Runtime version 11.6
== CUDA (ptxcompiler) [709] DEBUG -- Not patching Numba codegen

$ PTXCOMPILER_CHECK_NUMBA_CODEGEN_PATCH_NEEDED=1 PTXCOMPILER_APPLY_NUMBA_CODEGEN_PATCH=0 NUMBA_CUDA_LOG_LEVEL=DEBUG python -c "from ptxcompiler.patch import patch_numba_codegen_if_needed; patch_numba_codegen_if_needed()"
== CUDA (ptxcompiler) [295] DEBUG -- PTXCOMPILER_APPLY_NUMBA_CODEGEN_PATCH=0
== CUDA (ptxcompiler) [295] DEBUG -- PTXCOMPILER_CHECK_NUMBA_CODEGEN_PATCH_NEEDED=1
== CUDA (ptxcompiler) [671] DEBUG -- CUDA Driver version 11.7
== CUDA (ptxcompiler) [671] DEBUG -- CUDA Runtime version 11.6
== CUDA (ptxcompiler) [671] DEBUG -- Not patching Numba codegen

$ PTXCOMPILER_CHECK_NUMBA_CODEGEN_PATCH_NEEDED=0 PTXCOMPILER_APPLY_NUMBA_CODEGEN_PATCH=0 NUMBA_CUDA_LOG_LEVEL=DEBUG python -c "from ptxcompiler.patch import patch_numba_codegen_if_needed; patch_numba_codegen_if_needed()"
== CUDA (ptxcompiler) [294] DEBUG -- PTXCOMPILER_APPLY_NUMBA_CODEGEN_PATCH=0
== CUDA (ptxcompiler) [294] DEBUG -- PTXCOMPILER_CHECK_NUMBA_CODEGEN_PATCH_NEEDED=0
== CUDA (ptxcompiler) [295] DEBUG -- Not patching Numba codegen

$ PTXCOMPILER_CHECK_NUMBA_CODEGEN_PATCH_NEEDED=0 PTXCOMPILER_APPLY_NUMBA_CODEGEN_PATCH=1 NUMBA_CUDA_LOG_LEVEL=DEBUG python -c "from ptxcompiler.patch import patch_numba_codegen_if_needed; patch_numba_codegen_if_needed()"
== CUDA (ptxcompiler) [293] DEBUG -- PTXCOMPILER_APPLY_NUMBA_CODEGEN_PATCH=1
== CUDA (ptxcompiler) [293] DEBUG -- Patching Numba codegen for forward compatibility

$ PTXCOMPILER_CHECK_NUMBA_CODEGEN_PATCH_NEEDED=0 NUMBA_CUDA_LOG_LEVEL=DEBUG python -c "from ptxcompiler.patch import patch_numba_codegen_if_needed; patch_numba_codegen_if_needed()"
== CUDA (ptxcompiler) [294] DEBUG -- PTXCOMPILER_CHECK_NUMBA_CODEGEN_PATCH_NEEDED=0
== CUDA (ptxcompiler) [295] DEBUG -- Not patching Numba codegen

$ PTXCOMPILER_CHECK_NUMBA_CODEGEN_PATCH_NEEDED=1 NUMBA_CUDA_LOG_LEVEL=DEBUG python -c "from ptxcompiler.patch import patch_numba_codegen_if_needed; patch_numba_codegen_if_needed()"
== CUDA (ptxcompiler) [292] DEBUG -- PTXCOMPILER_CHECK_NUMBA_CODEGEN_PATCH_NEEDED=1
== CUDA (ptxcompiler) [669] DEBUG -- CUDA Driver version 11.7
== CUDA (ptxcompiler) [669] DEBUG -- CUDA Runtime version 11.6
== CUDA (ptxcompiler) [669] DEBUG -- Not patching Numba codegen

$ NUMBA_CUDA_LOG_LEVEL=DEBUG python -c "from ptxcompiler.patch import patch_numba_codegen_if_needed; patch_numba_codegen_if_needed()"
== CUDA (ptxcompiler) [670] DEBUG -- CUDA Driver version 11.7
== CUDA (ptxcompiler) [670] DEBUG -- CUDA Runtime version 11.6
== CUDA (ptxcompiler) [670] DEBUG -- Not patching Numba codegen

With this change, the `patch_needed()` function can also be used from other libraries without them needing to reimplement the logic and env vars to decide whether to check the driver and runtime versions.

wence-

I think the logic would be easier to read with my suggested changes (less double negation and tracking values over distance), but otherwise looks fine.

wence- · 2022-08-15T12:14:33Z

ptxcompiler/patch.py

+    if apply is not None:
+        logger.debug(f"PTXCOMPILER_APPLY_NUMBA_CODEGEN_PATCH={apply}")
+        try:
+            apply = int(apply)
+        except ValueError:
+            apply = False
+
+    if apply:
+        return True


Maybe:

Suggested change

if apply is not None:

logger.debug(f"PTXCOMPILER_APPLY_NUMBA_CODEGEN_PATCH={apply}")

try:

apply = int(apply)

except ValueError:

apply = False

if apply:

return True

if apply is not None:

logger.debug(f"PTXCOMPILER_APPLY_NUMBA_CODEGEN_PATCH={apply}")

try:

return int(apply)

except ValueError:

return False

I think this changes the behaviour so that the check won't be performed if the user sets PTXCOMPILER_APPLY_NUMBA_CODEGEN_PATCH to 0. Do we want to change this?

Ah yes, I see I changed the logic here too. I think we should leave as is. So APPLY=1 is a hard "definitely apply" and APPLY=0 is a soft "don't apply": first check if CHECK=1

wence- · 2022-08-15T12:18:46Z

ptxcompiler/patch.py

+    if check is not None:
+        logger.debug(f"PTXCOMPILER_CHECK_NUMBA_CODEGEN_PATCH_NEEDED={check}")
+        try:
+            check = int(check)
+        except ValueError:
+            check = False
+    else:
+        check = True
+
+    if not check:
+        return False


Suggested change

if check is not None:

logger.debug(f"PTXCOMPILER_CHECK_NUMBA_CODEGEN_PATCH_NEEDED={check}")

try:

check = int(check)

except ValueError:

check = False

else:

check = True

if not check:

return False

if check is not None:

logger.debug(f"PTXCOMPILER_CHECK_NUMBA_CODEGEN_PATCH_NEEDED={check}")

try:

return int(check)

except ValueError:

return False

If the user sets PTXCOMPILER_CHECK_NUMBA_CODEGEN_PATCH_NEEDED to 1, won't this end up always applying the patch instead of checking if the patch is needed?

Ah sorry, yes.

gmarkall · 2022-08-15T14:02:32Z

@wence- Many thanks for the review - comments / suggestions replied to.

bdice

Logic seems fine to me.

gmarkall · 2022-08-17T08:14:13Z

Many thanks all!

Move env var checks into patch_needed() function

529f7a5

With this change, the `patch_needed()` function can also be used from other libraries without them needing to reimplement the logic and env vars to decide whether to check the driver and runtime versions.

gmarkall requested a review from wence- August 15, 2022 09:12

wence- reviewed Aug 15, 2022

View reviewed changes

wence- approved these changes Aug 15, 2022

View reviewed changes

bdice approved these changes Aug 16, 2022

View reviewed changes

shwina approved these changes Aug 16, 2022

View reviewed changes

gmarkall merged commit 87c009e into rapidsai:main Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move env var checks into patch_needed() function #22

Move env var checks into patch_needed() function #22

gmarkall commented Aug 15, 2022

wence- left a comment

wence- Aug 15, 2022

gmarkall Aug 15, 2022

wence- Aug 15, 2022

wence- Aug 15, 2022

gmarkall Aug 15, 2022

wence- Aug 15, 2022

gmarkall commented Aug 15, 2022

bdice left a comment

gmarkall commented Aug 17, 2022

Move env var checks into patch_needed() function #22

Move env var checks into patch_needed() function #22

Conversation

gmarkall commented Aug 15, 2022

wence- left a comment

Choose a reason for hiding this comment

wence- Aug 15, 2022

Choose a reason for hiding this comment

gmarkall Aug 15, 2022

Choose a reason for hiding this comment

wence- Aug 15, 2022

Choose a reason for hiding this comment

wence- Aug 15, 2022

Choose a reason for hiding this comment

gmarkall Aug 15, 2022

Choose a reason for hiding this comment

wence- Aug 15, 2022

Choose a reason for hiding this comment

gmarkall commented Aug 15, 2022

bdice left a comment

Choose a reason for hiding this comment

gmarkall commented Aug 17, 2022