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

make sure nvtx3 is used #4937

Merged
merged 1 commit into from
Sep 16, 2024
Merged

make sure nvtx3 is used #4937

merged 1 commit into from
Sep 16, 2024

Conversation

jtrmal
Copy link
Contributor

@jtrmal jtrmal commented Sep 12, 2024

As far as I know, this is what nvToolsExt repo recommends (https://github.com/NVIDIA/NVTX) and they make it sound older version (sub-3) is ancient. Not sure how valid that claim is, but latest cuda docker (docker.io/nvidia/cuda:12.6.1-cudnn-devel-ubuntu22.04) needs this changes to compile -- otherwise it will end up complaining about mixing nvtx versions -- see https://github.com/jtrmal/kaldi/actions/runs/10830626409/job/30050811732#step:7:6551

I don't have much access to different grids/setups so not sure if this would be good enough or it will cause troubles -- in that case we would have to somehow detect nvtx3 and create define for conditional compiling. Or just get nvtx3 from github -- version 3 is header-only, so doesn't need any compilation and then linkage with a resulting library

TLDR: official docker image nvidia/cuda:12.6.1-cudnn-devel-ubuntu22.04 doesn't compile without this

@jtrmal
Copy link
Contributor Author

jtrmal commented Sep 13, 2024

I verified kaldi compiles with this patch even on nvidia/cuda:11.8.0-cudnn8-devel-ubuntu20.0 (generation older drivers).
So it should be fine -- feel free to chime in, but I plan to merge soon-ish (early next week) to be able to progress on the other work that is blocked by this.

@jtrmal
Copy link
Contributor Author

jtrmal commented Sep 16, 2024

I'll merge this and we will see what happens :)

@jtrmal jtrmal merged commit 22f36e5 into kaldi-asr:master Sep 16, 2024
1 of 2 checks passed
@danpovey
Copy link
Contributor

we got that same failure with lattice-utils-test.

@jtrmal
Copy link
Contributor Author

jtrmal commented Sep 16, 2024 via email

@danpovey
Copy link
Contributor

It looks like it might possibly be a failure with OpenFST's ShortestPath algorithm with n > 1 (i.e. requesting more than one path in n-best). Does anyone have time to see whether they made any changes to that algorithm from 1.7.x to 1.8.x?

@jtrmal
Copy link
Contributor Author

jtrmal commented Sep 17, 2024 via email

@danpovey
Copy link
Contributor

I tried to have a look at diffs of the OpenFst code but don't see anything that looks very relevant. It will probably be necessary to see the FSTs in the failing test.

@jtrmal
Copy link
Contributor Author

jtrmal commented Sep 17, 2024 via email

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.

2 participants