Uh oh!
There was an error while loading. Please reload this page.
Updated the redistributed version of Tensorflow to 1.14 - #3929
Conversation
Merging to personal fork
Syncing upstream fork
Checked in a better fix based on code review (#3896)
Syncing upstream fork
harishsk
commented
Jun 28, 2019
I have probable cause for why Mac tests are failing. The 1.13 package for Mac used to have .so files that we would rename to .dylib files in our build process. As part of the rename, it looks like we were also registering the path in the load library search path. I don't yet know whey the Linux tests are failing. |
harishsk
commented
Jul 1, 2019
A longer explanation of the fix is necessary. However, in v1.14.0 the naming scheme and archive contents have changed as follows:
In the list above, the files without a version number, sym-link to the files with a version. The fix has been complicated by the linux versions not using the .so suffix for their files and using the version number instead. If we only copy the files with version numbers, the tests fail because they are looking for a file with the standard extension. If we copy only the files with standard extension, the tests fail because the libtensorflow.so file attempts to load the libtensorflow_framework.so.1 file internally and doesn't find it. So we have to copy all the files. And further, all these files have to be copied to the directories that the tests run from so that they can find them locally. The ideal fix is to extract the files from the archive and just add that directory to the system's library search path. This is easier on Linux and Mac but a bit more complicated on Windows. I am stopping this here, because the current proposal is to migrate tensorflow redist package to the TensorFlow.NET repo. |
| <TensorFlowConfig Include="darwin" FileExtension=".tar.gz" FilesFromArchive="lib\libtensorflow.so;lib\libtensorflow_framework.so;include\tensorflow\c\LICENSE" Runtime="osx-x64" /> | ||
| <AdditionalDownloadFile Include="https://raw.githubusercontent.com/tensorflow/tensorflow/master/LICENSE" DownloadFile="$(IntermediateOutputPath)LICENSE" /> | ||
| <TensorFlowConfig Include="linux" FileExtension=".tar.gz" |
There was a problem hiding this comment.
Just an FYI -
The tests in the repo do not run against the produced NuGet packages. Instead, you need to manually test that the NuGet packages are created correctly.
Can you build the packages locally, and then manually smoke test them to ensure they are created correctly?
| <NativeAssemblyReference> | ||
| <FullAssemblyPath>$(NativeOutputPath)$(LibPrefix)%(NativeAssemblyReference.Identity)$(LibExtension)</FullAssemblyPath> | ||
| <!-- Tensorflow has a different naming scheme for v1.14.0. Those binaries need to be copied along with the standard names --> | ||
| <AssemblyPathWithMajorVersion Condition="'$(OS)' == 'Windows_NT'">$(NativeOutputPath)$(LibPrefix)%(NativeAssemblyReference.Identity)$(LibExtension)</AssemblyPathWithMajorVersion> |
There was a problem hiding this comment.
How is this different than the FullAssemblyPath above it? It looks like we'd be trying to copy the same file twice.
There was a problem hiding this comment.
Yes, for Windows, it ends up copying the same file twice. Without that line, I get an error in Windows builds that AssemblyPathMajorVersion is undefined.
I guess the other way to fix this is to make the optional copy conditional on the OS being linux or mac. Let me try that.
There was a problem hiding this comment.
If you want, you can try adding another section:
<NativeAssemblyReferenceCondition="'%(NativeAssemblyReference.MajorVersion)' != ''">
<!-- Tensorflow has a different naming scheme for v1.14.0. Those binaries need to be copied along with the standard names -->
<AssemblyPathWithMajorVersionCondition="'$(OS)' != 'Windows_NT'">$(NativeOutputPath)$(LibPrefix)%(NativeAssemblyReference.Identity)$(LibExtension).%(NativeAssemblyReference.MajorVersion)</AssemblyPathWithMajorVersion>
<AssemblyPathWithMajorVersionCondition="$([MSBuild]::IsOSPlatform('osx'))">$(NativeOutputPath)$(LibPrefix)%(NativeAssemblyReference.Identity).%(NativeAssemblyReference.MajorVersion)$(LibExtension)</AssemblyPathWithMajorVersion>
</NativeAssemblyReference>And then lower in your copy statement:
Condition="'%(AssemblyPathWithMajorVersion)' != '' AND Exists('%(AssemblyPathWithMajorVersion)')">
eerhardt
left a comment
There was a problem hiding this comment.
It would be good to test the produced NuGet packages separately to ensure they still work correctly. Just a simple smoke test that loads up a TF model and executes it should suffice.
harishsk
commented
Jul 8, 2019
I have tested the Nuget packages separately by having the dotnet/machinelearning samples reference the locally built nuget packages. |
* Fixed build errors resulting from upgrade to VS2019 compilers * Added additional message describing the previous fix * Updated Tensorflow to version 1.14 * Removed rename rules for Mac * Updated names of files for Mac * Attempting to fix test errors on Mac * Attempting to fix Mac test errors * Changed list of files to copy from archive for Linux and Mac * Fixed CopyNativeAssemblies to copy the tensorflow binaries that have version number appended * Fixed windows build * Another attempt at fixing both Windows and Linux builds * Changed copying version-suffixed binaries to occur only for non-Windows platforms * Changed copying of versioned assemblies to use HasMetadata
* Fixed build errors resulting from upgrade to VS2019 compilers * Added additional message describing the previous fix * Updated Tensorflow to version 1.14 * Removed rename rules for Mac * Updated names of files for Mac * Attempting to fix test errors on Mac * Attempting to fix Mac test errors * Changed list of files to copy from archive for Linux and Mac * Fixed CopyNativeAssemblies to copy the tensorflow binaries that have version number appended * Fixed windows build * Another attempt at fixing both Windows and Linux builds * Changed copying version-suffixed binaries to occur only for non-Windows platforms * Changed copying of versioned assemblies to use HasMetadata
fixes#3962