Skip to content

Bump version and simplify patches#61

Merged
h-vetinari merged 10 commits into
conda-forge:mainfrom
mmcauliffe:simplify-patches
Aug 2, 2025
Merged

Bump version and simplify patches#61
h-vetinari merged 10 commits into
conda-forge:mainfrom
mmcauliffe:simplify-patches

Conversation

@mmcauliffe

Copy link
Copy Markdown
Contributor

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-admin

conda-forge-admin commented Jul 30, 2025

Copy link
Copy Markdown
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe/meta.yaml:

  • ℹ️ The recipe is not parsable by parser conda-souschef (grayskull). This parser is not currently used by conda-forge, but may be in the future. We are collecting information to see which recipes are compatible with grayskull.
  • ℹ️ The recipe is not parsable by parser conda-recipe-manager. The recipe can only be automatically migrated to the new v1 format if it is parseable by conda-recipe-manager.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/16696642382. Examine the logs at this URL for more detail.

@h-vetinari

Copy link
Copy Markdown
Member

Thanks for tackling this! I had some commits in this direction in #54, feel free to take what's useful from there (if anything)

@mmcauliffe

Copy link
Copy Markdown
Contributor Author

@h-vetinari Making progress but a couple of questions for you!

  1. The linux builds are failing due to missing libnvToolsExt.so, I'm not sure why. The paths for CUDA_TOOLKIT_ROOT_DIR from 20a190a, didn't have it, so I tried to borrow how it was being set in torchaudio: https://github.com/conda-forge/torchaudio-feedstock/blob/main/recipe/build.sh, but it's still not finding libnvToolsExt.so. Do you have any ideas?
  2. Some of the selector logic you had kaldi v5.5.1168 #54 was causing build failures on osx, i.e. 398a39b caused it still want to try to install cuda dependencies, even though cuda_compiler_version was None. Would it be a good idea to migrate to the recipe.yaml like https://github.com/conda-forge/torchaudio-feedstock/blob/main/recipe/recipe.yaml? I assume that will remove the linter warnings about parsing, as well.

@h-vetinari

Copy link
Copy Markdown
Member

Hey @mmcauliffe

I'm travelling without a laptop at the moment, so I have to go mostly by memory. I remember struggling with the nvtools stuff in #48 and #53, perhaps some of the discussion there holds some useful nuggets of information. At least we're not changing CUDA version here, so this should be working as before, at least from the POV of our infrastructure (but I don't know how much the upstream CMake stuff changed).

The selector thing is minor, I wouldn't take this as the reason to translate to v1 type recipe yet. I mean, that should happen eventually, but independently (whether before or after) of the version bump.

@h-vetinari

Copy link
Copy Markdown
Member

Using the path-based search of the conda-forge metadata app, you can find that the library is present in cuda-nvtx-dev.

That library is being installed here, but I think the Jinja guard is broken.

@h-vetinari

Copy link
Copy Markdown
Member

This makes no sense to me. The current pins as rendered in the variant configs is 12.6


which isn't being changed here, and yet the builds here pull in 12.9

    cuda-cccl_linux-64:          12.9.27-ha770c72_0       conda-forge
    cuda-crt-dev_linux-64:       12.9.86-ha770c72_2       conda-forge
    cuda-cudart:                 12.9.79-h5888daf_0       conda-forge
    cuda-cudart-dev:             12.9.79-h5888daf_0       conda-forge
    cuda-cudart-dev_linux-64:    12.9.79-h3f2d84a_0       conda-forge
    cuda-cudart-static:          12.9.79-h5888daf_0       conda-forge
    cuda-cudart-static_linux-64: 12.9.79-h3f2d84a_0       conda-forge
    cuda-cudart_linux-64:        12.9.79-h3f2d84a_0       conda-forge
    cuda-driver-dev:             12.9.79-h5888daf_0       conda-forge
    cuda-driver-dev_linux-64:    12.9.79-h3f2d84a_0       conda-forge
    cuda-nvrtc:                  12.9.86-h5888daf_0       conda-forge
    cuda-nvrtc-dev:              12.9.86-h5888daf_0       conda-forge
    cuda-nvtx:                   12.9.79-h5888daf_0       conda-forge
    cuda-nvtx-dev:               12.9.79-ha770c72_0       conda-forge
    cuda-profiler-api:           12.9.79-h7938cbb_0       conda-forge
    cuda-version:                12.9-h4f385c5_3          conda-forge

@h-vetinari

Copy link
Copy Markdown
Member

And osx should not have an issue with using cuda_compiler_version; it's set correctly

cuda_compiler_version:
- None

@h-vetinari

Copy link
Copy Markdown
Member

Ok @mmcauliffe, this looks better now; the condition you added only had half of a tertiary expression, which apparently doesn't warn or error, but creates plenty of weirdness.

Feel free to rebase or squash my changes as you wish. The entire jinja block for cuda_major can also be deleted.

@mmcauliffe

Copy link
Copy Markdown
Contributor Author

Ahhhh ok, thanks for figuring that out!! I'll get this cleaned up!

@mmcauliffe mmcauliffe marked this pull request as ready for review August 2, 2025 20:32
@mmcauliffe

mmcauliffe commented Aug 2, 2025

Copy link
Copy Markdown
Contributor Author

@h-vetinari Ok this should be good to go if you want to take a last look over it!

I'll try to get a PR submitted to kaldi to incorporate the changes needed to get shared libraries building on Windows, that should help simplify the needed patches further and is probably generally useful upstream. But in the meantime, this should be good to merge in.

@h-vetinari h-vetinari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@h-vetinari h-vetinari merged commit 0b7585c into conda-forge:main Aug 2, 2025
13 checks passed
This was referenced Aug 2, 2025
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

Successfully merging this pull request may close these issues.

3 participants