Fix cquery-ing with cuda targets #209

mvukov · 2024-01-03T11:37:05Z

This makes possible to do the following:

cd examples
bazel cquery //if_cuda/... --@rules_cuda//cuda:enable=False

when one wants to run cquery (e.g. within bazel-diff) in an env without cuda (e.g. a CI).

I read the readme of if_cuda example. It's advertised that it's OK that bazel errors out if one want to build a cuda target when rules_cuda is disabled. I'd say that's a drastic measure. Folks should IMO mark their bazel targets like I did in this PR.

cloudhan

Otherwise, looks good to me.

cuda/private/toolchain_configs/dummy.bzl

cloudhan · 2024-01-04T01:26:47Z

It somehow don't work with bzlmod:

> bazel cquery //if_cuda/... --@rules_cuda//cuda:enable=False --enable_bzlmod
ERROR: no such package '@@[unknown repo 'platforms' requested from @@]//': The repository '@@[unknown repo 'platforms' requested from @@]' could not be resolved: No repository visible as '@platforms' from main repository
ERROR: Analysis of target '//if_cuda:kernel' failed; build aborted: no such package '@@[unknown repo 'platforms' requested from @@]//': The repository '@@[unknown repo 'platforms' requested from @@]' could not be resolved: No repository visible as '@platforms' from main repository
ERROR: Build did NOT complete successfully

mvukov · 2024-01-04T11:04:49Z

Fixed bzlmod for the disabled toolchain.
Updated docs

Also, handled the case when cuda is enabled but cuda toolkit cannot be found. The previous behavior was that bazel build would fail because some targets are not present in @local_cuda//*, and thats true because @local_cuda//:BUILD was empty. This was not quite obvious/ergonomic unless you would dig into the code and found out that the BUILD file was empty. I refactored the code such that when we want to build a cuda target as e.g.

bazel build //if_cuda:kernel --@rules_cuda//cuda:enable=true

but there is no cuda toolkit available we get

ERROR: Target //if_cuda:kernel is incompatible and cannot be built, but was explicitly requested.
Dependency chain:
    //if_cuda:kernel (9867ef)   <-- target platform (@local_config_platform//:host) didn't satisfy constraint @platforms//:incompatible
FAILED: Build did NOT complete successfully (37 packages loaded, 138 targets configured)

This is IMO more readable, with addition of the docs for requires_cuda helper.

Thinking a bit further, it makes more sense to use globally @rules_cuda//cuda:is_enabled_and_cuda_found instead of @rules_cuda//cuda:is_enabled. But I'd like to hear your opinion on this. I'm open for different naming of is_enabled_and_cuda_found :) Maybe we can just rename is_enabled -> _is_enabled and is_enabled_and_cuda_found -> is_enabled?

mvukov · 2024-01-04T11:05:02Z

@cloudhan PTAL.

mvukov · 2024-01-04T11:07:55Z

cuda/private/defs.bzl

@@ -0,0 +1,16 @@
+"""private defs"""
+
+def requires_cuda():


Taken and adapted from https://github.com/tensorflow/runtime, there used to be rules_cuda repo in there.

mvukov · 2024-01-04T11:10:51Z

examples/if_cuda/BUILD.bazel


 cuda_library(
    name = "kernel",
    srcs = ["kernel.cu"],
    hdrs = ["kernel.h"],
+    target_compatible_with = requires_cuda(),


Just a side note: users could still get non-so-obvious errors from bazel in case the target compatibility is not set.

cuda/private/defs.bzl

cloudhan · 2024-01-04T13:16:24Z

Thinking a bit further, it makes more sense to use globally @rules_cuda//cuda:is_enabled_and_cuda_found instead of @rules_cuda//cuda:is_enabled. But I'd like to hear your opinion on this.

I'd prefer not to add it. Because it somehow to me, it mixed the target and host, the enable generally means the bazel build invocation generate targets that is able to run cuda code (so you enable it). The toolchain part, however, should be regarded as the host where bazel runs is able to produce that target.

In the case of the toolchain being mis-configured or not configured, it is a host_compatiable_with thing (the attribute does not exist!).

@jsharpe Any comment?

mvukov · 2024-01-04T13:34:37Z

I looked into how "@platforms//:incompatible" is defined and made a change in d72a951.

bazel build //if_cuda:kernel   --@rules_cuda//cuda:enable=false
ERROR: Target //if_cuda:kernel is incompatible and cannot be built, but was explicitly requested.
Dependency chain:
    //if_cuda:kernel (6b3a99)   <-- target platform (@local_config_platform//:host) didn't satisfy constraints [@rules_cuda//cuda:cuda_must_be_enabled, @rules_cuda//cuda:cuda_must_be_found]
FAILED: Build did NOT complete successfully (35 packages loaded, 139 targets configured)

The second constraint is a bit misleading because we didn't even look for it. Open to suggestions.

bazel build //if_cuda:kernel   --@rules_cuda//cuda:enable=true
ERROR: Target //if_cuda:kernel is incompatible and cannot be built, but was explicitly requested.
Dependency chain:
    //if_cuda:kernel (9867ef)   <-- target platform (@local_config_platform//:host) didn't satisfy constraint @rules_cuda//cuda:cuda_must_be_found
FAILED: Build did NOT complete successfully (0 packages loaded, 137 targets configured)

For the reference: ⬆️ is for the case cuda is enabled but cuda toolkit is not found.

@cloudhan PTAL.

mvukov · 2024-01-16T16:03:18Z

@cloudhan friendly reminder: PTAL. I implemented changes you requested.

cloudhan · 2024-01-16T16:08:58Z

Sorry for the delay, was dealing with the CI update for Bazel 7 support.

cloudhan · 2024-01-16T16:22:41Z

Let me play around with it to see if we can simplify it a little bit.

cloudhan · 2024-01-17T01:48:45Z

To be honest, what is dragging me down in these changes is the latter maneuver that tries to hide error info

... No matching toolchains found for types @rules_cuda//cuda:toolchain_type.
To debug, rerun with --toolchain_resolution_debug='@rules_cuda//cuda:toolchain_type'

It doesn't change the situation (that toolchain is not found), but add another layer of indirection.

IIRC the initial change fixed cquery-ing error and looks good...

mvukov · 2024-01-17T08:36:02Z

I don't know what to do about your last comment. Can you propose an alternative? As-is, folks can't use cquery when there is no cuda toolchain present. This PR fixes that situation and eventually gives a more user-friendly failure message IMO.

mvukov · 2024-01-17T08:38:55Z

Are you not happy about requires_cuda() macro logic but all the rest seems OK? If so, I can remove that macro and I'll have that in my monorepo.

cloudhan · 2024-01-17T11:13:35Z

The _requires_cuda_found() part (aka, the constraint on cuda toolkit is found) . It hides the unresolved toolchain error with a custom constraint. Thus the builtin No matching toolchains found for types @rules_cuda//cuda:toolchain_type error is substituted with didn't satisfy constraints ... @rules_cuda//cuda:cuda_must_be_found

Better split it into a seperate PR? I might ask some bazel toolchain experts on what is the best practice for it...

cloudhan · 2024-01-16T16:17:07Z

cuda/BUILD.bazel

+
+constraint_setting(name = "cuda_must_be_enabled_setting")
+
+constraint_value(
+    name = "cuda_must_be_enabled",
+    constraint_setting = ":cuda_must_be_enabled_setting",
+)
+
+constraint_setting(name = "cuda_must_be_found_setting")
+
+constraint_value(
+    name = "cuda_must_be_found",
+    constraint_setting = ":cuda_must_be_found_setting",
+)


If you split this PR, please also address the naming

In the cuda namespace (//cuda:), you don't repeat cuda, then you can see //cuda:must_be_enabled and //cuda:must_be_found are extremely confusing. I'd suggest renaming these as rules_are_enabled... and valid_toolchain_is_configured or something the like.

Fixed naming.

cloudhan · 2024-01-16T16:19:17Z

cuda/private/defs.bzl

+    * CUDA is enabled and
+    * CUDA toolchain is found.


* rules are enabled and * toolchain is found.

cloudhan · 2024-01-16T16:19:34Z

cuda/private/defs.bzl

+    the conditions are not satisfied. Incompatible targets are excluded
+    from bazel target wildcards and fail to build if requested explicitly.
+    """
+    return _requires_is_enabled() + _requires_cuda_found()


mvukov · 2024-01-19T15:39:35Z

The _requires_cuda_found() part (aka, the constraint on cuda toolkit is found) . It hides the unresolved toolchain error with a custom constraint. Thus the builtin No matching toolchains found for types @rules_cuda//cuda:toolchain_type error is substituted with didn't satisfy constraints ... @rules_cuda//cuda:cuda_must_be_found

So, with this PR, the unresolved toolchain error is gone, because a dummy one is actually registered. If we remove target_compatible_with = requires_cuda(), from //if_cuda:kernel, then we get now:

bazel build //if_cuda:kernel 
Starting local Bazel server and connecting to it...
ERROR: /home/milan/.cache/bazel/_bazel_milan/d4ba7c041b1396ebf4926b6ac8cc612b/external/rules_cuda/cuda/BUILD.bazel:90:11: every rule of type label_flag implicitly depends upon the target '@local_cuda//:cuda_runtime', but this target could not be found because of: no such target '@local_cuda//:cuda_runtime': target 'cuda_runtime' not declared in package '' defined by /home/milan/.cache/bazel/_bazel_milan/d4ba7c041b1396ebf4926b6ac8cc612b/external/local_cuda/BUILD (Tip: use `query "@local_cuda//:*"` to see all the targets in that package)
ERROR: Analysis of target '//if_cuda:kernel' failed; build aborted: 
FAILED: Build did NOT complete successfully (40 packages loaded, 139 targets configured)

if cuda toolchain is not found. Moreover, cquery still doesn't work:

bazel cquery //if_cuda:*
ERROR: /home/milan/.cache/bazel/_bazel_milan/d4ba7c041b1396ebf4926b6ac8cc612b/external/rules_cuda/cuda/BUILD.bazel:90:11: every rule of type label_flag implicitly depends upon the target '@local_cuda//:cuda_runtime', but this target could not be found because of: no such target '@local_cuda//:cuda_runtime': target 'cuda_runtime' not declared in package '' defined by /home/milan/.cache/bazel/_bazel_milan/d4ba7c041b1396ebf4926b6ac8cc612b/external/local_cuda/BUILD (Tip: use `query "@local_cuda//:*"` to see all the targets in that package)
ERROR: Analysis of target '//if_cuda:kernel' failed; build aborted: 
FAILED: Build did NOT complete successfully (0 packages loaded, 2 targets configured)

So, for cquery to work, one must use target_compatible_with (IMO, this is good practice).

I made an update to this PR such that both cquery work + that we get IMO more dev-friendly errors than @local_cuda//:cuda_runtime is missing.

WDYT? Let's first sort this out and then I can address your remarks.

cloudhan · 2024-01-19T16:44:35Z

OK, I see your point now. The situation is caused by the ":disabled-local-toolchain" being relying on the target_settings = [":cuda_is_disabled"],. I tried to make it always available as a bottom case, but then the toolchain will ask for a compiler_executable from the toolchain config.

The compiler_executable is a string (mirrored from CcToolchainInfo), whereas it should have been a Label so that we can build a dummy one from dummy.cc, I am not sure if this is possible tho...

Lets adopt the current change to fix the issue. Please address naming tho.

mvukov · 2024-01-25T13:58:30Z

@cloudhan PTAL, I addressed your comments.

Fix cquery-ing with cuda targets

4d86e4c

mvukov requested review from cloudhan, jsharpe and ryanleary as code owners January 3, 2024 11:37

cloudhan requested changes Jan 4, 2024

View reviewed changes

cuda/private/toolchain_configs/dummy.bzl Outdated Show resolved Hide resolved

mvukov added 4 commits January 4, 2024 08:22

Add none toolchain to bzlmod config

5af0739

Streamline naming for disabled toolchain

4d3f530

Clean up config_disabled

86e9888

Handle case then cuda is not found but enabled

8e0814b

mvukov commented Jan 4, 2024

View reviewed changes

cloudhan reviewed Jan 4, 2024

View reviewed changes

cuda/private/defs.bzl Outdated Show resolved Hide resolved

Add custom constraint values for error handling

d72a951

Update docs

4d631a3

cloudhan reviewed Jan 17, 2024

View reviewed changes

Fix naming

0714ff6

cloudhan approved these changes Jan 25, 2024

View reviewed changes

cloudhan merged commit 3f24292 into bazel-contrib:main Jan 25, 2024
14 checks passed

cloudhan mentioned this pull request Aug 8, 2024

fix(bzlmod): allow both root module and our module to call cuda.local_toolchain #264

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cquery-ing with cuda targets #209

Fix cquery-ing with cuda targets #209

mvukov commented Jan 3, 2024

cloudhan left a comment

cloudhan commented Jan 4, 2024

mvukov commented Jan 4, 2024 •

edited

Loading

mvukov commented Jan 4, 2024

mvukov Jan 4, 2024

mvukov Jan 4, 2024

cloudhan commented Jan 4, 2024

mvukov commented Jan 4, 2024 •

edited

Loading

mvukov commented Jan 16, 2024

cloudhan commented Jan 16, 2024

cloudhan commented Jan 16, 2024

cloudhan commented Jan 17, 2024 •

edited

Loading

mvukov commented Jan 17, 2024

mvukov commented Jan 17, 2024

cloudhan commented Jan 17, 2024

cloudhan Jan 16, 2024

mvukov Jan 25, 2024

cloudhan Jan 16, 2024

mvukov Jan 25, 2024

cloudhan Jan 16, 2024

mvukov Jan 25, 2024

mvukov commented Jan 19, 2024 •

edited

Loading

cloudhan commented Jan 19, 2024 •

edited

Loading

mvukov commented Jan 25, 2024

Fix cquery-ing with cuda targets #209

Fix cquery-ing with cuda targets #209

Conversation

mvukov commented Jan 3, 2024

cloudhan left a comment

Choose a reason for hiding this comment

cloudhan commented Jan 4, 2024

mvukov commented Jan 4, 2024 • edited Loading

mvukov commented Jan 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudhan commented Jan 4, 2024

mvukov commented Jan 4, 2024 • edited Loading

mvukov commented Jan 16, 2024

cloudhan commented Jan 16, 2024

cloudhan commented Jan 16, 2024

cloudhan commented Jan 17, 2024 • edited Loading

mvukov commented Jan 17, 2024

mvukov commented Jan 17, 2024

cloudhan commented Jan 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvukov commented Jan 19, 2024 • edited Loading

cloudhan commented Jan 19, 2024 • edited Loading

mvukov commented Jan 25, 2024

mvukov commented Jan 4, 2024 •

edited

Loading

mvukov commented Jan 4, 2024 •

edited

Loading

cloudhan commented Jan 17, 2024 •

edited

Loading

mvukov commented Jan 19, 2024 •

edited

Loading

cloudhan commented Jan 19, 2024 •

edited

Loading