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

Torchaudio #18331

Closed
wants to merge 23 commits into from
Closed

Torchaudio #18331

wants to merge 23 commits into from

Conversation

thewchan
Copy link
Contributor

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • 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).
  • If static libraries are linked in, the license of the static library is packaged.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

@conda-forge-linter
Copy link

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 (recipes/torchaudio) and found it was in an excellent condition.

@thewchan
Copy link
Contributor Author

@conda-forge/staged-recipes this is likely going to need a lot of work and help to get working... Any help folks can provide highly appreciated! @conda-forge/torchvision (I borrowed heavily from yall's recipe)

@hmaarrfk
Copy link
Contributor

I really think split recipes introduce unecessary complexity at this stage.

I would suggest removing it

@thewchan
Copy link
Contributor Author

@hmaarrfk Thanks; I'll try that!

@h-vetinari
Copy link
Member

I had started with this in #17082. Problem that I ran into was conda-forge/kaldi-feedstock#15 (though the issue is also that the kaldi-feedstock currently has a blas setup that's quite incompatible with the rest of conda-forge), which I'm attempting to fix in conda-forge/kaldi-feedstock#25. I've gotten the non-windows builds going, but could use some help with windows.

@hmaarrfk
Copy link
Contributor

You are hitting a problem where they expect you to be using git to build
https://github.com/pytorch/audio/tree/main/third_party/kaldi

You can use git, this is somewhat frowned upon, but I had to ensure maintainability of pytorch.

For 1 package, you might be able to pull exactly the right version and keep things in sync.
https://github.com/conda-forge/staged-recipes/pull/17620/files#diff-18d4986353d397c3b27d7366f7bfa374de054aa475730a8e71dc0d5f09f29ac6R39

Then you should make some noise upstream to both torchaudio and kaldi to release and use real version numbers. They don't have incentives unless they know their users will use them and it brings benefits to them.

@thewchan
Copy link
Contributor Author

@hmaarrfk Thanks for the guidance. It looks like they are using a custom build for Kaldi; would I be able to use git to pull that version of Kaldi from the torchaudio repo? Is that something doable?

@hmaarrfk
Copy link
Contributor

Step #1, get things to build.
Step #2. Get things approved.

Do what you need at this stage.

You might also find this useful: https://github.com/conda-forge/pytorch-cpu-feedstock/blob/main/recipe/meta.yaml

@thewchan
Copy link
Contributor Author

@hmaarrfk is there a way in meta.yaml to specify the commit hash of a git repo to pull? I can only find how to do this with tags or versions?

@hmaarrfk
Copy link
Contributor

@thewchan
Copy link
Contributor Author

Hmm this is some strange C++ error that I'm not familiar with... @conda-forge/help-c-cpp

@thewchan
Copy link
Contributor Author

Running into this error even after I added libblas as a build dependencies

CMake Error at /home/conda/staged-recipes/build_artifacts/torchaudio_1647365684713/_build_env/share/cmake-3.22/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find BLAS (missing: BLAS_LIBRARIES)

@hmaarrfk
Copy link
Contributor

@hmaarrfk
Copy link
Contributor

https://github.com/conda-forge/pytorch-cpu-feedstock/blob/main/recipe/conda_build_config.yaml#L2

@h-vetinari
Copy link
Member

It looks like they are using a custom build for Kaldi; would I be able to use git to pull that version of Kaldi from the torchaudio repo? Is that something doable?

... did you see/read my comment further up?

@thewchan
Copy link
Contributor Author

@h-vetinari, yes, can you help me understand how I'd apply what you did in this case?

@h-vetinari
Copy link
Member

@h-vetinari, yes, can you help me understand how I'd apply what you did in this case?

It'd be possible to move forward with a unix-only version of conda-forge/kaldi-feedstock#25, but there shouldn't be the assumption that just by virtue of restarting from scratch, you'll automatically overcome the blockers that someone else already identified. Obviously, help on those blockers would be appreciated.

@thewchan
Copy link
Contributor Author

@conda-forge-admin, please restart ci

@thewchan
Copy link
Contributor Author

@conda-forge-admin, please restart ci

@janjagusch janjagusch mentioned this pull request Feb 2, 2023
10 tasks
@stale
Copy link

stale bot commented Apr 13, 2023

Hi friend!

We really, really, really appreciate that you have taken the time to make a PR on conda-forge/staged-recipes! conda-forge only exists because people like you donate their time to build and maintain conda recipes for use by the community.

In an effort to maintain this repository and increase the signal-to-noise for open PRs, the maintainers of staged-recipes close excessively old PRs after six months. This PR will remain open for another month, and then will be closed.

If you'd like to keep it open, please comment/push and we will be happy to oblige! Note that very old PRs will likely need to be rebased on main so that they can be rebuilt with the most recent CI scripts. If you have any trouble, or we missed reviewing this PR in the first place (sorry!), feel free to ping the team using a special command in a comment on the PR to get the attention of the staged-recipes team.

Cheers and thank you for contributing to this community effort!

@stale stale bot added the stale will be closed in 30 days label Apr 13, 2023
@stale
Copy link

stale bot commented May 13, 2023

Hi again! About a month ago, we commented on this PR saying it would be closed in another month if it was still inactive. It has been a month and so now it is being closed. Thank you so much for making it in the first place and contributing to the community project that is conda-forge. If you'd like to reopen this PR, please feel free to do so at any time!

Cheers and have a great day!

@stale stale bot closed this May 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants