Testing 1.8.2 PR changes #38

mmcauliffe · 2023-04-12T18:21:02Z

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.

Testing out changes from kaldi-asr/kaldi#4829 that updates Kaldi to use OpenFST 1.8.2

conda-forge-webservices · 2023-04-12T18:21:08Z

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

mmcauliffe · 2023-04-13T21:41:42Z

@h-vetinari ok this should be basically working! Kaldi dlls are built for windows, and it uses OpenFST from conda forge now. Massive patch as it incorporates the changes in kaldi-asr/kaldi#4829 as well as fixes on top. I've submitted my changes to georgthegreat/kaldi#1, so hopefully once those get merged in we can get the OpenFST 1.8.2 PR merged.

h-vetinari · 2023-04-14T00:24:57Z

Haven't reviewed the patches but that sounds great, thanks a lot @mmcauliffe! :)

mmcauliffe · 2023-04-28T16:07:22Z

@h-vetinari Since there hasn't been any movement on either of the upstream PRs, I'm wondering if you could take a look over this? https://github.com/mmcauliffe/kaldi/tree/conda-shared is the branch that the patches are based on.

I have a couple of projects that have dependencies on this. I've been working on pybind11 bindings for the shared libraries and the basic functionality is working for calling Kaldi feature generation and decoding graph compilation functions from Python, so it'd be nice to get that released and integrated into MFA.

h-vetinari · 2023-05-09T05:53:48Z

Hey @mmcauliffe, sorry I missed this ping in my flood of notifications 😑

h-vetinari · 2023-05-09T06:08:56Z

Looking at mmcauliffe/kaldi@2d2e057, I have to wonder if all those typdef changes are really necessary? They seem pretty harmless, and blow up the patch for no good reason. That said: whatever.

I'm not a huge fan of the following:

#ifdef kaldi_base_BUILT_AS_STATIC
#  define kaldi_base_EXPORT
#  define KALDI_base_NO_EXPORT
#else
#ifdef _WIN32
#  ifndef kaldi_base_EXPORT
#    ifdef kaldi_base_EXPORTS
        /* We are building this library */
#      define kaldi_base_EXPORT __declspec(dllexport)
#    else
        /* We are using this library */
#      define kaldi_base_EXPORT __declspec(dllimport)
#    endif
#  endif

#  ifndef KALDI_BASE_NO_EXPORT
#    define KALDI_BASE_NO_EXPORT 
#  endif
#else
        #define kaldi_base_EXPORT
#endif // _WIN32
#endif

for various reasons:

naming the macro kaldi_base_EXPORT when it needs to switch between export and import
the distinction between kaldi_base_EXPORT & kaldi_base_EXPORTS (can you see it at first glance?)
the dependence on magic CMake variables that one has to know about
the inconsistent indentation

I prefer to be explicit by setting the required symbols explicitly. I did this recently for another windows DLL-ification project (with the mechanism based on what abseil is doing), which would look like

#if defined(KALDI_DLL_EXPORTS)
#  define KALDI_DLL __declspec(dllexport)
#elif defined(KALDI_DLL_IMPORTS)
#  define KALDI_DLL __declspec(dllimport)
#else
#  define KALDI_DLL
#endif // defined(KALDI_DLL_EXPORTS)

where the key CMake addition is

if(BUILD_SHARED_LIBS)
  set_target_properties(<target> PROPERTIES DEFINE_SYMBOL "KALDI_DLL_EXPORTS")
  target_compile_definitions(<target> INTERFACE KALDI_DLL_IMPORTS)
endif()

By putting that in the interface, each consuming library automatically gets the right __declspec, without having to define KALDI_DLL_IMPORTS itself.

mmcauliffe · 2023-05-09T17:40:15Z

Looking at mmcauliffe/kaldi@2d2e057, I have to wonder if all those typdef changes are really necessary? They seem pretty harmless, and blow up the patch for no good reason. That said: whatever.

Yeah, they're a consequence of the changes here: https://github.com/kaldi-asr/kaldi/pull/4829/files#diff-338cced3fed5be3eaa511ced41fe5d8d2ddf826d0468f2d8c06edb62f3f6d042L42 where typedefs were moved from the Kaldi namespace to the global one. There's some discussion of it kaldi-asr/kaldi#4829 but not resolved, so I just went with the new version, but if you think it's better just to revert the scoping of the type defs and keep the typedef kaldi::int32 int32; throughout to minimize the patch, I'm ok with that too. I don't really have an opinion one way or the other.

I'm not a huge fan of the following:

#ifdef kaldi_base_BUILT_AS_STATIC
#  define kaldi_base_EXPORT
#  define KALDI_base_NO_EXPORT
#else
#ifdef _WIN32
#  ifndef kaldi_base_EXPORT
#    ifdef kaldi_base_EXPORTS
        /* We are building this library */
#      define kaldi_base_EXPORT __declspec(dllexport)
#    else
        /* We are using this library */
#      define kaldi_base_EXPORT __declspec(dllimport)
#    endif
#  endif

#  ifndef KALDI_BASE_NO_EXPORT
#    define KALDI_BASE_NO_EXPORT 
#  endif
#else
        #define kaldi_base_EXPORT
#endif // _WIN32
#endif

for various reasons:

* naming the macro `kaldi_base_EXPORT` when it needs to switch between export _and_ import

* the distinction between `kaldi_base_EXPORT` & `kaldi_base_EXPORTS` (can you see it at first glance?)

* the dependence on magic CMake variables that one has to know about

* the inconsistent indentation

I prefer to be explicit by setting the required symbols explicitly. I did this recently for another windows DLL-ification project (with the mechanism based on what abseil is doing), which would look like

#if defined(KALDI_DLL_EXPORTS)
#  define KALDI_DLL __declspec(dllexport)
#elif defined(KALDI_DLL_IMPORTS)
#  define KALDI_DLL __declspec(dllimport)
#else
#  define KALDI_DLL
#endif // defined(KALDI_DLL_EXPORTS)

where the key CMake addition is

if(BUILD_SHARED_LIBS)
  set_target_properties(<target> PROPERTIES DEFINE_SYMBOL "KALDI_DLL_EXPORTS")
  target_compile_definitions(<target> INTERFACE KALDI_DLL_IMPORTS)
endif()

By putting that in the interface, each consuming library automatically gets the right __declspec, without having to define KALDI_DLL_IMPORTS itself.

That does seem cleaner, I was basing the original exports header file based on what cmake was generating from https://cmake.org/cmake/help/latest/module/GenerateExportHeader.html, but I'll do it this way since it seems a lot less confusing.

h-vetinari · 2023-05-09T22:53:21Z

so I just went with the new version, but if you think it's better just to revert the scoping of the type defs and keep the typedef kaldi::int32 int32; throughout to minimize the patch, I'm ok with that too.

From what I can tell by looking at the linked diff, there should be no functional difference and it's "just" a question of consistency? If so, that doesn't meet the level for inclusion into the patch here IMO, especially not if we get thousands of lines of raw patch file that drown out the relevant changes. So if you're already reworking this, I'd prefer to drop those changes. If however it's necessary from a functional POV (e.g. it doesn't compile without, or would produce the wrong results), then go ahead and leave them in.

mmcauliffe · 2023-05-11T00:31:13Z

@h-vetinari ok, got everything working again, rolled back the typedef changes and used the template you had for DLL exports. One thing I ran into is that I had to explicitly pass the relevant DLL_IMPORT definition to cuda_compile for some reason, it wasn't getting picked up somehow when linking to the library with the symbols.

h-vetinari

Starting to look very good. Just some nits & questions

recipe/meta.yaml

h-vetinari · 2023-05-11T00:24:16Z

recipe/patches/0004-Windows-shared-libs.patch

+diff --git a/src/base/kaldi-types.h b/src/base/kaldi-types.h
+index 68d5578..3a1b8b9 100644
+--- a/src/base/kaldi-types.h
+++ b/src/base/kaldi-types.h
+@@ -39,16 +39,30 @@ typedef float   BaseFloat;
+ // we find in the future lacks stdint.h
+ #include <stdint.h>
+
+-typedef int8_t   int8;
+-typedef int16_t  int16;
+-typedef int32_t  int32;
+-typedef int64_t  int64;


This part seems to be duplicated with patch 3. Probably it passes because patch recognizes it has already been applied...?

Yeah I did one patch for the 1.82 changes on the original PR and then another patch on top of those with my changes to get it working, just to keep it separate, but getting rid of the typedefs meant that I undid what was done in the original PR.

h-vetinari · 2023-05-11T00:26:22Z

recipe/patches/0004-Windows-shared-libs.patch

+   // should call CuDevice::Instantiate().SelectGpuId(..).
+   static CuDevice& Instantiate() {
+-    CuDevice &ans = this_thread_device_;
+    CuDevice &ans = this_thread_device();


Ah, it seems you picked up a few tricks from the gprc work? ;-)

Or if you found out how to deal with thread_local vs. __declspec(dllexport) independently, I doff my hat!

Yes, I meant to mention! I noticed you had a workaround in a follow up commit, so used that here, no hat doffing required.

I didn't come up with that workaround myself either, I had been completely stuck (hence I'd have been impressed if you had come up with it by yourself 🙃)

recipe/patches/0004-Windows-shared-libs.patch

recipe/run_test.py

recipe/meta.yaml

recipe/run_test.py

h-vetinari

Alright, this LGTM! Thanks so much! :)

h-vetinari · 2023-05-11T19:02:23Z

The one thing I'd like to avoid is having the multiple thousand-line patch churn in the git history. Would you mind rebasing the PR to clean up the history a bit, or are you fine with a squash merge? I'm also willing to do something manually to separate the recipe rerendering from your commits (an intelligent squash merge, if you will).

mmcauliffe · 2023-05-11T19:03:40Z

I usually squash merge, so if that's alright with you, I can just do that for this one too.

h-vetinari · 2023-05-11T20:39:37Z

Go ahead! :)

Testing 1.8.2 PR changes

6a7b529

mmcauliffe added 7 commits April 12, 2023 11:29

Fix patch

15346d7

Fix patches

d5bf258

Add openfst dependencies

e4804b0

Bump CXX_STANDARD to 17

f7ef7fa

Update windows build patch

84e457b

Fix bug in patch for non-Windows

c4df726

Update tests and remove linux cuda 11.1 builds due to bug in 11.1

d261482

Fix patch file

9e482fd

mmcauliffe mentioned this pull request Apr 14, 2023

[DO NOT MERGE] Support openfst 1.8.2 kaldi-asr/kaldi#4829

Closed

mmcauliffe marked this pull request as ready for review May 9, 2023 00:30

mmcauliffe requested review from h-vetinari and mcernak as code owners May 9, 2023 00:30

mmcauliffe added 3 commits May 10, 2023 13:39

Simplify patch

e33e891

Fix bug for non-Windows cuda compilation

c90d4ac

Fix bug in patch for non-global int32

a5f7066

h-vetinari reviewed May 11, 2023

View reviewed changes

mmcauliffe added 2 commits May 10, 2023 20:05

Add support for online bins and online dll

d4f900f

Update patches

a79eea7

h-vetinari reviewed May 11, 2023

View reviewed changes

recipe/meta.yaml Outdated Show resolved Hide resolved

h-vetinari reviewed May 11, 2023

View reviewed changes

recipe/run_test.py Outdated Show resolved Hide resolved

Update tests

f0e6d4b

mmcauliffe requested a review from h-vetinari May 11, 2023 17:41

h-vetinari approved these changes May 11, 2023

View reviewed changes

mmcauliffe merged commit ce3d6bf into conda-forge:main May 11, 2023

mmcauliffe deleted the 182_support branch May 11, 2023 20:59

h-vetinari mentioned this pull request May 15, 2023

switch to dynamic libraries also on windows #31

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Testing 1.8.2 PR changes #38

Testing 1.8.2 PR changes #38

mmcauliffe commented Apr 12, 2023

conda-forge-webservices bot commented Apr 12, 2023

mmcauliffe commented Apr 13, 2023

h-vetinari commented Apr 14, 2023

mmcauliffe commented Apr 28, 2023

h-vetinari commented May 9, 2023

h-vetinari commented May 9, 2023 •

edited

Loading

mmcauliffe commented May 9, 2023

h-vetinari commented May 9, 2023

mmcauliffe commented May 11, 2023

h-vetinari left a comment

h-vetinari May 11, 2023

mmcauliffe May 11, 2023

h-vetinari May 11, 2023

mmcauliffe May 11, 2023

h-vetinari May 11, 2023

h-vetinari left a comment

h-vetinari commented May 11, 2023 •

edited

Loading

mmcauliffe commented May 11, 2023

h-vetinari commented May 11, 2023

Testing 1.8.2 PR changes #38

Testing 1.8.2 PR changes #38

Conversation

mmcauliffe commented Apr 12, 2023

conda-forge-webservices bot commented Apr 12, 2023

mmcauliffe commented Apr 13, 2023

h-vetinari commented Apr 14, 2023

mmcauliffe commented Apr 28, 2023

h-vetinari commented May 9, 2023

h-vetinari commented May 9, 2023 • edited Loading

mmcauliffe commented May 9, 2023

h-vetinari commented May 9, 2023

mmcauliffe commented May 11, 2023

h-vetinari left a comment

Choose a reason for hiding this comment

h-vetinari May 11, 2023

Choose a reason for hiding this comment

mmcauliffe May 11, 2023

Choose a reason for hiding this comment

h-vetinari May 11, 2023

Choose a reason for hiding this comment

mmcauliffe May 11, 2023

Choose a reason for hiding this comment

h-vetinari May 11, 2023

Choose a reason for hiding this comment

h-vetinari left a comment

Choose a reason for hiding this comment

h-vetinari commented May 11, 2023 • edited Loading

mmcauliffe commented May 11, 2023

h-vetinari commented May 11, 2023

h-vetinari commented May 9, 2023 •

edited

Loading

h-vetinari commented May 11, 2023 •

edited

Loading