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

Fix k2 and torchaudio installation (Docker, macOS) #6094

Merged
merged 10 commits into from
Mar 1, 2023

Conversation

artbataev
Copy link
Collaborator

@artbataev artbataev commented Feb 23, 2023

What does this PR do?

  • add build arguments REQUIRE_TORCHAUDIO and REQUIRE_K2 for torchaudio and k2 in Dockerfile
    • false (by default): library will be installed if possible, errors ignored
    • true: if library not installed correctly, end build with error
  • fix k2 installation script on macOS
  • fix k2 installation in Docker (k2 depends on torchaudio)
  • fix torchaudio installation for latest PyTorch versions
  • also fixes bug https://nvbugswb.nvidia.com/NvBugs5/SWBug.aspx?bugid=3940510&cmtNo= (apt update && apt install -y libavdevice-dev should be also used before executing install script)

NB: This PR doesn't fix k2 + Cuda 12.0 compilation errors.

Collection: [CI]

Changelog

  • k2 installation script is compatible with macOS
  • k2 installation script fails if error occurred during installation
  • Dockerfile: add build arguments (see description above)
  • torchaudio installation script - fixes for latest PyTorch versions (ffmpeg, match release branch, correct version)

Usage

# build container, ignore k2/torchaudio installation error
docker build --tag "nemo:latest" .

# ignore k2 installation error, fail on torchaudio installation error
docker build --tag "nemo:latest" --build-arg REQUIRE_TORCHAUDIO=true --build-arg REQUIRE_K2=false .

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests?
  • Did you add or update any necessary documentation?
  • Does the PR affect components that are optional to install? (Ex: Numba, Pynini, Apex etc)
    • Reviewer: Does the PR have correct import guards for all optional libraries?

PR Type:

  • New Feature
  • Bugfix
  • Documentation

If you haven't finished some of the above items you can still open "Draft" PR.

Who can review?

Anyone in the community is free to review the PR once the checks have passed.
Contributor guidelines contains specific people who can review PRs to various areas.

Additional Information

  • Related to # (issue)

@github-actions github-actions bot added the CI label Feb 23, 2023
Signed-off-by: Vladimir Bataev <[email protected]>
Signed-off-by: Vladimir Bataev <[email protected]>
@artbataev artbataev changed the title Dockerfile: flags for k2 and torchaudio. Temporary make torchaudio optional Dockerfile: flags for k2 and torchaudio Feb 23, 2023
@artbataev artbataev changed the title Dockerfile: flags for k2 and torchaudio Fix for k2 and torchaudio installation (Docker, macOS) Mar 1, 2023
@artbataev artbataev changed the title Fix for k2 and torchaudio installation (Docker, macOS) Fix k2 and torchaudio installation (Docker, macOS) Mar 1, 2023
Signed-off-by: Vladimir Bataev <[email protected]>
Signed-off-by: Vladimir Bataev <[email protected]>
Signed-off-by: Vladimir Bataev <[email protected]>
Signed-off-by: Vladimir Bataev <[email protected]>
@artbataev artbataev requested a review from GNroy March 1, 2023 16:03
@artbataev artbataev marked this pull request as ready for review March 1, 2023 16:03
Copy link
Collaborator

@GNroy GNroy left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Collaborator

@titu1994 titu1994 left a comment

Choose a reason for hiding this comment

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

Thanks for this fix !

@titu1994 titu1994 merged commit 9a700ac into NVIDIA:main Mar 1, 2023
artbataev added a commit to artbataev/NeMo that referenced this pull request Mar 1, 2023
titu1994 pushed a commit that referenced this pull request Mar 1, 2023
titu1994 pushed a commit to titu1994/NeMo that referenced this pull request Mar 24, 2023
* torchaudio and k2: add flags, make optional

Signed-off-by: Vladimir Bataev <[email protected]>

* Fix syntax

Signed-off-by: Vladimir Bataev <[email protected]>

* Fix return code

Signed-off-by: Vladimir Bataev <[email protected]>

* Fix k2 installation

Signed-off-by: Vladimir Bataev <[email protected]>

* Fix torchaudio + k2 installation in docker

Signed-off-by: Vladimir Bataev <[email protected]>

* Fix flags

Signed-off-by: Vladimir Bataev <[email protected]>

* Fix torchaudio installation

Signed-off-by: Vladimir Bataev <[email protected]>

* Fix branch ckeck

Signed-off-by: Vladimir Bataev <[email protected]>

* Fix latest torchaudio installation

Signed-off-by: Vladimir Bataev <[email protected]>

---------

Signed-off-by: Vladimir Bataev <[email protected]>
hsiehjackson pushed a commit to hsiehjackson/NeMo that referenced this pull request Jun 2, 2023
* torchaudio and k2: add flags, make optional

Signed-off-by: Vladimir Bataev <[email protected]>

* Fix syntax

Signed-off-by: Vladimir Bataev <[email protected]>

* Fix return code

Signed-off-by: Vladimir Bataev <[email protected]>

* Fix k2 installation

Signed-off-by: Vladimir Bataev <[email protected]>

* Fix torchaudio + k2 installation in docker

Signed-off-by: Vladimir Bataev <[email protected]>

* Fix flags

Signed-off-by: Vladimir Bataev <[email protected]>

* Fix torchaudio installation

Signed-off-by: Vladimir Bataev <[email protected]>

* Fix branch ckeck

Signed-off-by: Vladimir Bataev <[email protected]>

* Fix latest torchaudio installation

Signed-off-by: Vladimir Bataev <[email protected]>

---------

Signed-off-by: Vladimir Bataev <[email protected]>
Signed-off-by: hsiehjackson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants