Closed
Conversation
github-actions
bot
added
6.topic: cuda
github-actions
bot
added
10.rebuild-darwin: 1-10
SomeoneSerge
force-pushed
the
refactor/cuda/db/draft
branch
from
c5c1cc7 to
792b8ab
Compare
nix-owners
bot
requested review from
ConnorBaker,
alyssais,
knedlsepp and
samuela
SomeoneSerge
force-pushed
the
refactor/cuda/db/draft
branch
from
792b8ab to
5d11cf6
Compare
This was referenced May 13, 2025
SomeoneSerge
force-pushed
the
refactor/cuda/db/draft
branch
from
5d11cf6 to
559d767
Compare
13 tasks
Contributor
Author
|
I'm consistently getting the feeling that checking in the unnormalized
JSONs from Nvidia and having the default Nixpkgs instance rely on the
obviously fragile parsing logic is a mistake...
|
SomeoneSerge
force-pushed
the
refactor/cuda/db/draft
branch
from
d0155bb to
956ec27
Compare
github-actions
bot
added
the
11.by: package-maintainer
SomeoneSerge
commented
May 19, 2025
SomeoneSerge
commented
May 19, 2025
SomeoneSerge
commented
May 19, 2025
SomeoneSerge
force-pushed
the
refactor/cuda/db/draft
branch
from
7f6fb51 to
bfc2851
Compare
Certain pieces of software (wrap, trt) published by nvidia are published without an actual license text granting any explicit rights, but with an spdxId in e.g. headers; public comments suggest that this is a catch-all copyright notice; I propose we model this as a separate entry in lib.licenses, explicitly citing the spdxId, and separate from cuda licenses
SomeoneSerge
force-pushed
the
refactor/cuda/db/draft
branch
3 times, most recently
from
98853b8 to
a46025c
Compare
Adds `tests.cuda.db`, `tests.cuda.db.html`, and `_cuda.db`. The commit obtained certain mass moves after rebasing on NixOS#406531. The original motivation behind using evalModules in cudaPackages was to ensure that the Data used to generate the package set can be evaluated (& inspected) in its entirety. This focus was lost in implementation Various stylistic and design choices: - Use of all-singular names. - Predominantly column-oriented layout (structure of arrays rather than array of structures); exceptions have to do with rebasings and compatibility. - Column validation: tried domains assertions vs. submodule with dynamic option sets; chose the former to get the more concise <name> placeholders in the nixosOptionsDoc output. - Between Maybe T and Option<T> ended up choosing the former (consistent with Connor's code). - Instead of AttrSet and attrsOf, chose to use infix notation (`String ⇒ T) with the bold arrow hinting at a "memoized" map (cf. dex-lang). Performance considerations: - Adding FOD info to the schema originally contributed extra 1.5s to evaluation time because of `imports` (`mkMerge`) abuse: `json.nix` generates many small definitions, most of which are redundant are are retained in `options.<path>.definitions`. - In the worst case, at the time of writing, unoptimized `json.nix` results in 4s evaluation time when used with the full set of manifests, including the new backported manifests which doubles the number of FODs and versions compared to Nixpkgs. - For this reason we do not directly use `json.nix` in Nixpkgs, but we export and check in the output of `evalModules` (cf. _rawManifests in the next PRs), bringing down the evaluation time to hundreds of miliseconds. It's obviously desirable to optimize this further, but at least this doesn't make things worse. On checking-in vendored content. The output of evalModules is only smaller (measured in lines of code) than the total size of upstream manifests. Most of the reduction comes from deduplicating the "feature manifests".
Dropping pre-manifest (< 11.4) cudaPackages as unsupported, but will recover them falling back to the run-file for the backport Move cudaPackages.backendStdenv's passthruExtra to _cuda/lib/flags.nix to ensure flags do not depend on the current scope but only on parent. Includes many review suggestions from: Co-authored-by: Connor Baker <connor.baker@tweag.io>
Downloads all JSON manifests published by NVIDIA at known locations using recursive wget. Only manifests declared in advance are kept to make the outputHash (more) stable. Passthru features a database derived from the FOD's JSONs, using IFD. Further, `cudaPackages._rawManifests.exportNew` can be (ab)used for updates: it exports a (slightly) stripped version of the database split into `blobs.json` and `packages.json`, which can be passed to _cuda/db's `extraModules` via `importJSON`.
Instead use JSONs generated by `nix-build -A cudaPackages._rawManifests.exportNew`
SomeoneSerge
force-pushed
the
refactor/cuda/db/draft
branch
from
a46025c to
72ed55c
Compare
Contributor
|
Contributor
|
Disclaimer: this was done on macOS Tahoe so it may just be the developer beta breaking things. I'll try to reproduce on a different system later.
|
Contributor
|
For what it's worth, with derivation 'collectx_bringup-1.19.13' has invalid meta attribute 'license'
derivation 'collectx_bringup-1.19.13' has invalid meta attribute 'license'
derivation 'cuda_cccl-11.8.89' has invalid meta attribute 'license'
derivation 'cuda_cccl-12.8.90' has invalid meta attribute 'license'
derivation 'cuda_cudart-11.8.89' has invalid meta attribute 'license'
derivation 'cuda_cudart-12.8.90' has invalid meta attribute 'license'
derivation 'cuda_cuobjdump-11.8.86' has invalid meta attribute 'license'
derivation 'cuda_cuobjdump-12.8.90' has invalid meta attribute 'license'
derivation 'cuda_cupti-11.8.87' has invalid meta attribute 'license'
derivation 'cuda_cupti-12.8.90' has invalid meta attribute 'license'
derivation 'cuda_cuxxfilt-11.8.86' has invalid meta attribute 'license'
derivation 'cuda_cuxxfilt-12.8.90' has invalid meta attribute 'license'
derivation 'cuda_demo_suite-11.8.86' has invalid meta attribute 'license'
derivation 'cuda_demo_suite-12.8.90' has invalid meta attribute 'license'
derivation 'cuda_documentation-11.8.86' has invalid meta attribute 'license'
derivation 'cuda_documentation-12.8.90' has invalid meta attribute 'license'
derivation 'cuda_gdb-11.8.86' has invalid meta attribute 'license' |
Contributor
|
I'm able to reproduce the ELF magic number error on a different Darwin system (which is running release software instead of the developer betas); maybe it's worth restricting the supported platforms for |
ConnorBaker
reviewed
Jun 10, 2025
Contributor
ConnorBaker
left a comment
ConnorBaker
left a comment
There was a problem hiding this comment.
Lookin' good! Left a few more comments -- feel free to ping me whenever you're ready for another round :)
Contributor
There was a problem hiding this comment.
Why is this change included in this PR?
Comment on lines
+56
to
+59
| "source" = listToSetOfStr lib.platforms.all; | ||
| "linux-aarch64"."aarch64-linux" = unit; | ||
| "linux-ppc64le"."powerpc64le-linux" = unit; | ||
| "linux-all" = listToSetOfStr lib.platforms.linux; |
Contributor
There was a problem hiding this comment.
I'd recommend restricting these to just linux platforms, or even further (e.g., no MIPS or RISC-V).
Comment on lines
+64
to
+72
| errors = builtins.concatMap ( | ||
| { assertion, message }: lib.optionals (!assertion) [ message ] | ||
| ) evaluated.config.assertions; | ||
| error = if errors == [ ] then null else lib.concatStringsSep "\n" errors; |
Contributor
There was a problem hiding this comment.
_mkFailedAssertionsString:
You can then check if the resulting string is empty and go based on that (since it doesn't look like you're keeping errors around for anything in particular.
Comment on lines
+6
to
+8
| inherit (import ./columnar.nix) | ||
| unit | ||
| ; |
Contributor
There was a problem hiding this comment.
Suggested change
| inherit (import ./columnar.nix) | |
| unit | |
| ; | |
| inherit (import ./columnar.nix) unit; |
| name = lib.mapAttrs (pname: _: lib.mkDefault pname) attrs; | ||
| systemsNv = lib.mapAttrs (pname: { releases }: lib.mapAttrs (_: _: unit) releases) attrs; | ||
| }; | ||
| release.product = lib.mapAttrs (pname: _: unit) attrs; |
Contributor
There was a problem hiding this comment.
Is this meant to be equivalent to package.pname?
Comment on lines
+62
to
+68
| nextVersion = | ||
| version: | ||
| let | ||
| ints = map lib.toInt (lib.splitVersion version); | ||
| intsNext = lib.take (lib.length ints - 1) ints ++ [ (lib.last ints + 1) ]; | ||
| in | ||
| lib.concatMapStringsSep "." toString intsNext; |
Contributor
There was a problem hiding this comment.
Suggested change
| nextVersion = | |
| version: | |
| let | |
| ints = map lib.toInt (lib.splitVersion version); | |
| intsNext = lib.take (lib.length ints - 1) ints ++ [ (lib.last ints + 1) ]; | |
| in | |
| lib.concatMapStringsSep "." toString intsNext; | |
| nextVersion = | |
| version: | |
| let | |
| components = lib.splitVersion version; | |
| componentsNext = lib.init components ++ [ (toString (lib.toInt (lib.last components) + 1)) ]; | |
| in | |
| lib.concatStringsSep "." componentsNext; |
You only need to convert the last component to an integer to do arithmetic to it.
Worth adding a note that this function assumes non-calendar versioning schemes where we can arbitrarily add one to the final version component.
Comment on lines
+11
to
+13
| inherit (import ./columnar.nix) | ||
| unit | ||
| ; |
Contributor
There was a problem hiding this comment.
Suggested change
| inherit (import ./columnar.nix) | |
| unit | |
| ; | |
| inherit (import ./columnar.nix) unit; |
Comment on lines
+53
to
+60
| StrByLength = mkOptionType { | ||
| name = "StrByLength"; | ||
| description = "string (merged by choosing longest)"; | ||
| descriptionClass = "noun"; | ||
| check = lib.isString; | ||
| merge = mergeByLength lib.stringLength; | ||
| }; |
Contributor
There was a problem hiding this comment.
Ah, okay.
Do you happen to remember any examples of conflicting values from their JSON? I'm interested, and I think Kevin would be as well.
nixpkgs-ci
bot
added
the
2.status: merge conflict
This was referenced Sep 10, 2025
13 tasks
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/change-package-version-tensorrt/70146/8 |
Contributor
Author
|
Need to revisit the approach & tradeoffs again in view of #437723 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Towards improving out-of-tree customization experience of
cudaPackages(related: #406531)The idea is to postpone the generation of
cudaPackagesinstances until after we've produced a completely static and fully-evaluated "database", accounting for information from all of the JSON manifests, the conflicts already resolved, and missing data filled in. This "database" being static means we should be able to convert it to JSON without instantiating any package sets.With such a static "database", package set generation should be completely trivial. "Trivial" should mean, for example, that we never see "missing attribute" errors or empty platforms again: #282185 #276800 #406207 #406668
The current aim is to reproduce the existing package set with this two-stage approach.
In future, however, I anticipate completely eliminating the distinction between "redistributable releases" and the legacy "runfile releases": if each "downloadable artifact" (i.e. a content hash) is associated with a list of "packages" (tuple
pname,platform,gencodes,version,tags?) it contains, and the generic builder is capable of determining the location of components requested by the derivation, then there's no reason we could not generate acuda_cudart(marked non-redistributable) from the unpacked.runcontainer.Example JSON produced by
nix-build -A tests.cuda.db:submodule) https://cuda-index.someonex.net/static/frhf9msl851dsjfmsnvq9qxjn2c9di56-cudb-options.htmlCC @ConnorBaker
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.