add torchaudio - Githubissues

hadim commented 1 month ago

Checklist

[x] Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
[x] License file is packaged (see here for an example).
[x] Source is from official source.
[x] Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
[x] If static libraries are linked in, the license of the static library is packaged.
[x] Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
[x] Build number is 0.
[x] A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
[x] GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
[x] When in trouble, please check our knowledge base documentation before pinging a team.

hadim commented 1 month ago

Trying to revamp https://github.com/conda-forge/staged-recipes/pull/17082

github-actions[bot] commented 1 month ago

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

I Failed to even lint the recipe, probably because of a conda-smithy bug :cry:. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint --conda-forge . from the recipe directory.

_{This message was generated by GitHub actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/11267788945.}

hadim commented 1 month ago

@conda-forge/help-c-cpp ready for review

github-actions[bot] commented 1 month ago

Hi! This is the staged-recipes linter and your PR looks excellent! :rocket:

hadim commented 1 month ago

@hmaarrfk LGTM for me, but please let me know if you think something is missing.

hmaarrfk commented 1 month ago

Looks fine, but to be honest, I'm not sure on where we stand with rattler recipes. Are we accepting them yet? or just building out infrastructure for them?

h-vetinari commented 1 month ago

Looks fine, but to be honest, I'm not sure on where we stand with rattler recipes. Are we accepting them yet?

Yes we are (as of https://github.com/conda-forge/conda-forge-ci-setup-feedstock/pull/337). There's a few already

Tobias-Fischer commented 1 month ago

This might help as a temporary workaround for the ffmpeg issues: https://github.com/pytorch/audio/pull/3837

hadim commented 1 month ago

Thank you @Tobias-Fischer. What I don't get is why ffmpeg is not detected during the tests.

I won't have the time to wrap this PR in the short term, so if anyone needs it, go ahead. Otherwise, I might come back to it at a later time.

Tobias-Fischer commented 1 month ago

Any chance to give me write access to your fork @hadim?

hadim commented 1 month ago

Any chance to give me write access to your fork @hadim?

Should be good now.

Tobias-Fischer commented 1 month ago

@hmaarrfk @h-vetinari - do you think it would be acceptable if we disable ffmpeg integration? I saw it’s disabled in the torchvision feedstock. Same for sox which seems to cause issues.

hmaarrfk commented 1 month ago

Yes. The torchvision ffmpeg integration is stuck on ffmpeg 4 and was always broken IMO.

It is extremely hard to get right.

I don't really know how well they tested it upstream.

h-vetinari commented 1 month ago

Yeah, fine by me to disable. Sox had some other issues (there's some patching involved that upstream explicitly ruled out IIRC), some references can be found in my old PR and on the sox feedstock

hadim commented 1 month ago

Well done @Tobias-Fischer. It looks like it's working.

The tests take about one hour per variant. Are you sure you want to take much time for building the package? I would maybe suggest downsizing the number of tests to be run. That would remove some pressure on the c-f CI but also reduce the testing loop of the feedstock itself.

But it's really up to you, and it's just a suggestion!

(also, do you mind removing me from the maintainer list, as I don't think I'll have the bandwidth to maintain it in the future?)

hmaarrfk commented 1 month ago

I'm not sure if this kind of test skip strategy is preferred to you: https://github.com/conda-forge/scikit-image-feedstock/blob/main/recipe/meta.yaml#L73

Tobias-Fischer commented 1 month ago

@hadim: Thanks for the feedback! The stress on CIs should not be too bad once it's a feedstock, as separate python versions will be built in separate CI jobs. That said, I'll temporarily use just a single python version from my next commit onward to reduce the stress here.

@hmaarrfk: Yes, the current solution is ugly. I don't know yet how we can do something like the strategy you linked to with the new recipe.yaml. @wolfv - do you have a suggestion?

hmaarrfk commented 1 month ago

Oh. I guess just maybe write one test per line in the test script?

that way we can read and understand each skip line by line.

h-vetinari commented 1 month ago

For test skips, I definitely prefer the approach used in skimage (and elsewhere), where each component of the not (x or y or a) string we're building gets a comment or reference about why that test us skipped

Tobias-Fischer commented 1 month ago

For test skips, I definitely prefer the approach used in skimage (and elsewhere), where each component of the not (x or y or a) string we're building gets a comment or reference about why that test us skipped

Isn’t that what we do now already? We group them into different failure cases.

Tobias-Fischer commented 1 month ago

@h-vetinari @hmaarrfk would you like to become maintainers as well?

hmaarrfk commented 1 month ago

no ;)

h-vetinari commented 1 month ago

Given the work I did on this in https://github.com/conda-forge/staged-recipes/pull/17082, sure.

wolfv commented 1 month ago

Yeah there are some secret ways that could make something like this possible ... I am not yet sure if I like it or not! :)

Note: this doesn't seem to work with the version of minijinja we are currently using, but I can try to fix that asap:

context:
  tests: |
    ${{ 
      (
        (['test1', 'test2', 'test3'] if ppc64le else []) +
        ['test1', 'test2']
      ) | join(" ")
    }}

This would render to a (string) list. I am unsure about comments in Jinja expressions.

Otherwise one could also argue to write a more explicit "script" either in Python or in Bash that would give the list of tests to run. Downside: it's not in the recipe itself, upside it is a proper programming language.

wolfv commented 1 month ago

If the tests run only on Unix you could also use something like:

script:
  - echo "foo" > tests.txt
  # run tests because bla bla
  - if: ppc64le
    then:
      - echo "bar baz bing" >> tests.txt
  - pytest bla $(cat tests.txt)

As another "saner" alternative. On Windows, sadly, syntax is always a bit different.

Options are:

use nushell
use python
wait for us to finish https://github.com/prefix-dev/shell :)

wolfv commented 1 month ago

Argh, sorry, acidentally slipped and closed the PR! Apologies 😞

wolfv commented 1 month ago

And if you really want (although I am not sure we will keep this "feature") you can do:

context:
  fuuu: |
    {% set bla = "xxx" %}
    ${{ bla | upper }}

h-vetinari commented 1 month ago

although I am not sure we will keep this "feature"

This kind of freedom would be very useful in some of the more complicated recipes that you've mostly descoped so far (AFAIU), because you care primarily about the bulk of simpler recipes for now. Is {% ... %} still "pure YAML"? I cannot find it in the spec.

In any case, I'd like to not lose the ability to do commented test skips (at least talking about pytest -k) within the recipe. Whether that works as before with some variables that get extended or whether the skips get saved into a skips.xyz file (or whatever) doesn't matter to me, but I want it to be obvious from the recipe that there are test skips, what they are exactly, and the reasoning for each skip in a comment.

wolfv commented 1 month ago

@h-vetinari yes, it is pure YAML because it is contained in a multi-line text block. :)

Also when I tested, the setted variable did not escape "outside" so maybe this can be allowed.

Tobias-Fischer commented 1 month ago

Does anyone have an idea why it can’t find nvcc for cuda 12? It looks in PREFIX instead of BUILD_PREFIX despite setting the suggested environment variable CUDACXX

Tobias-Fischer commented 1 month ago

I think this is finally ready for review :)

@hadim: Could you please remove the Draft label from the PR?

Tobias-Fischer commented 1 month ago

Note that cuda 11.8 failed with "No space left on device (os error 28)" which will not be an issue once it's converted to a feedstock with separate builds.

Tobias-Fischer commented 1 month ago

Thanks @hmaarrfk for the review!

conda-forge / staged-recipes

add torchaudio #27715