Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring common aarch64 & x86_64 items in dependencies.yaml #585

Closed
jakirkham opened this issue Jul 26, 2023 · 6 comments
Closed

Refactoring common aarch64 & x86_64 items in dependencies.yaml #585

jakirkham opened this issue Jul 26, 2023 · 6 comments

Comments

@jakirkham
Copy link
Member

Filing an issue to track this suggestion from @vyasr

#572 (comment)

@jakirkham
Copy link
Member Author

Note this requires work in rapidsai/dependency-file-generator. Maybe addressing issue ( rapidsai/dependency-file-generator#41 ) or similar would help

@bdice
Copy link
Contributor

bdice commented Jul 26, 2023

I don't know if there's a clear action item here. We've discussed some possible new features for rapids-dependency-file-generator but I don't think that partially matching multiple matrices (the proposed change as I understand from this comment) is consistent with our current design for the dependencies schema. I would vote to close this, since I believe the approach we settled on in #572 is largely consistent with our current intent for the dependencies design despite the slightly awkward repetition.

@bdice
Copy link
Contributor

bdice commented Jul 26, 2023

The alternative I would suggest is replacing the existing matrix with two separate matrices as shown below to separate things that are dependent on arch and CUDA version from those dependent only on CUDA version. However, this doesn't reduce the lines of code, so I am not a strong proponent of this.

Current state:

      - output_types: conda
        matrices:
          - matrix:
              arch: x86_64
              cuda: "11.8"
            packages:
              - cuda-version=11.8
              - nvcc_linux-64=11.8
              - libcufile=1.4.0.31
              - libcufile-dev=1.4.0.31
              - libnvjpeg=11.6.0.55
              - libnvjpeg-dev=11.6.0.55
          - matrix:
              arch: aarch64
              cuda: "11.8"
            packages:
              - cuda-version=11.8
              - nvcc_linux-aarch64=11.8
              - libnvjpeg=11.6.0.55
              - libnvjpeg-dev=11.6.0.55
          - matrix:
              cuda: "12.0"
            packages:
              - cuda-version=12.0
              - cuda-nvcc
              - libcufile-dev
              - libnvjpeg-dev
              - libnvjpeg-static

Proposed change:

      - output_types: conda
        matrices:
          - matrix:
              cuda: "11.8"
            packages:
              - cuda-version=11.8
              - libnvjpeg=11.6.0.55
              - libnvjpeg-dev=11.6.0.55
          - matrix:
              cuda: "12.0"
            packages:
              - cuda-version=12.0
              - libnvjpeg-dev
              - libnvjpeg-static
      - output_types: conda
        matrices:
          - matrix:
              arch: x86_64
              cuda: "11.8"
            packages:
              - nvcc_linux-64=11.8
              - libcufile=1.4.0.31
              - libcufile-dev=1.4.0.31
          - matrix:
              arch: aarch64
              cuda: "11.8"
            packages:
              - nvcc_linux-aarch64=11.8
          - matrix:
              cuda: "12.0"
            packages:
              - cuda-nvcc
              - libcufile-dev

@jakirkham
Copy link
Member Author

Think that was what Vyas was proposing. In any event think he tried this and it didn't work IIUC, but maybe there's something else we are missing. Feel free to give it a go 🙂

@vyasr
Copy link
Contributor

vyasr commented Jul 28, 2023

Bradley is correct, what I was originally proposing is partially matching multiple matrices and I don't think we actually plan to implement that. I did consider doing what Bradley's example does, but I decided against it since (as he noted) it doesn't really shorten much. It is technically more DRY, but not enough to be worth the change IMO. I think we can close this issue. The next time these requirement lists change again we can take a look and see if there are other opportunities to improve the lists.

@jakirkham
Copy link
Member Author

Ok sounds good. Thanks you both! 🙏

@jakirkham jakirkham closed this as not planned Won't fix, can't repro, duplicate, stale Jul 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants