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.

Sign up for GitHub

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

add string normalize method #1461

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+40 −6
Open

add string normalize method #1461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
587fa88
add string normalize method
alinpahontu2912 Mar 19, 2025
4fa0e34
remove unnecessary method
alinpahontu2912 Mar 26, 2025
681d2e3
Merge branch 'dotnet:main' into fix_dll_loading
alinpahontu2912 Mar 27, 2025
1ec4b10
improve exception messages, change method visibility and add unit test
alinpahontu2912 Mar 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 27 additions & 6 deletions src/TorchSharp/Torch.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,26 @@ public static partial class torch
static bool nativeBackendCudaLoaded = false;

public static string __version__ => libtorchPackageVersion;
public static string NormalizeNuGetVersion(string versionString)
{
if (string.IsNullOrWhiteSpace(versionString))
throw new ArgumentException($"Invalid NuGet version: {versionString}. Version string is null, empty or only contains whitespaces");

string[] parts = versionString.Split('-', '+');
string[] versionParts = parts[0].Split('.');

if (versionParts.Length < 2 || versionParts.Length > 4 || !versionParts.All(v => int.TryParse(v, out _)))
throw new ArgumentException($"Invalid NuGet version: {versionString}. Please check: https://learn.microsoft.com/en-us/nuget/concepts/package-versioning");

string normalizedVersion = versionParts[0] + "." + versionParts[1];
if (versionParts.Length > 2) normalizedVersion += "." + versionParts[2];
if (versionParts.Length > 3 && int.Parse(versionParts[3]) != 0) normalizedVersion += "." + versionParts[3];

if (parts.Length > 1)
normalizedVersion += "-" + parts[1];

return normalizedVersion;
}

internal static bool TryLoadNativeLibraryFromFile(string path, StringBuilder trace) {
bool ok;
Expand Down Expand Up @@ -168,16 +188,17 @@ private static void LoadNativeBackend(bool useCudaBackend, out StringBuilder? tr

if (torchsharpLoc!.Contains("torchsharp") && torchsharpLoc.Contains("lib") && Directory.Exists(packagesDir) && Directory.Exists(torchsharpHome)) {

var torchSharpVersion = Path.GetFileName(torchsharpHome); // really GetDirectoryName

var assembly = typeof(torch).Assembly;
var version = assembly.GetName().Version;
var torchSharpVersion = (version != null) ? version.ToString() : Path.GetFileName(torchsharpHome);
Copy link
Member Author

Choose a reason for hiding this comment

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

When taking the version string directly from the library it results in a 4 number version string: eg: 0.105.0.0, which needs to be normalized to remove last 0. I think it's better to have it taken straight from the dll that was loaded to the project than the nugget install folder.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, I would recommend to consider behavior change (reading via assembly) in a separated PR - issue.

For this PR, we can focus on the issue we have.

Later, we can do the other change (reading via assembly) with only that scope.

So, in case of any revert, we can keep the fix safe.

if (useCudaBackend) {
var consolidatedDir = Path.Combine(torchsharpLoc, $"cuda-{cudaVersion}");

trace.AppendLine($" Trying dynamic load for .NET/F# Interactive by consolidating native {cudaRootPackage}-* binaries to {consolidatedDir}...");

var cudaOk = CopyNativeComponentsIntoSingleDirectory(packagesDir, $"{cudaRootPackage}-*", libtorchPackageVersion, consolidatedDir, trace);
var cudaOk = CopyNativeComponentsIntoSingleDirectory(packagesDir, $"{cudaRootPackage}-*", NormalizeNuGetVersion(libtorchPackageVersion), consolidatedDir, trace);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of calling NormalizeNuGetVersion(libtorchPackageVersion) and NormalizeNuGetVersion(torchSharpVersion) two times in the argument level, could you please consider to move this normalization process to the just after libtorchPackageVersion and torchSharpVersion initialization?

With doing it:

  • We can prevent repetition
  • We wouldn't need to call NormalizeNuGetVersion for the future code expansion (in case we add something more at below)
  • Better readability => as NormalizeNuGetVersion method is relevant the versions , calling it just after the version initializations would make it easier to follow-up

if (cudaOk) {
cudaOk = CopyNativeComponentsIntoSingleDirectory(packagesDir, "torchsharp", torchSharpVersion, consolidatedDir, trace);
cudaOk = CopyNativeComponentsIntoSingleDirectory(packagesDir, "torchsharp", NormalizeNuGetVersion(torchSharpVersion), consolidatedDir, trace);
if (cudaOk) {
var consolidated = Path.Combine(consolidatedDir, target);
ok = TryLoadNativeLibraryFromFile(consolidated, trace);
Expand All @@ -193,9 +214,9 @@ private static void LoadNativeBackend(bool useCudaBackend, out StringBuilder? tr

trace.AppendLine($" Trying dynamic load for .NET/F# Interactive by consolidating native {cpuRootPackage}-* binaries to {consolidatedDir}...");

var cpuOk = CopyNativeComponentsIntoSingleDirectory(packagesDir, cpuRootPackage, libtorchPackageVersion, consolidatedDir, trace);
var cpuOk = CopyNativeComponentsIntoSingleDirectory(packagesDir, cpuRootPackage, NormalizeNuGetVersion(libtorchPackageVersion), consolidatedDir, trace);
if (cpuOk) {
cpuOk = CopyNativeComponentsIntoSingleDirectory(packagesDir, "torchsharp", torchSharpVersion, consolidatedDir, trace);
cpuOk = CopyNativeComponentsIntoSingleDirectory(packagesDir, "torchsharp", NormalizeNuGetVersion(torchSharpVersion), consolidatedDir, trace);
if (cpuOk) {
var consolidated = Path.Combine(consolidatedDir, target);
ok = TryLoadNativeLibraryFromFile(consolidated, trace);
Expand Down
13 changes: 13 additions & 0 deletions test/TorchSharpTest/TestTorchSharp.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
using System.Collections.Generic;
using System.Linq.Expressions;
using System.Reflection;
using System.Security.Cryptography;
using Tensorboard;
using Xunit;

using static TorchSharp.torch;
Expand Down Expand Up @@ -450,6 +452,17 @@ public void AllowFP16ReductionCuBLAS()
Assert.False(torch.backends.cuda.matmul.allow_fp16_reduced_precision_reduction);
}

[Fact]
public void CheckVersionStrings()
{
Assert.Equal("2.5.1", torch.NormalizeNuGetVersion("2.5.1.0"));
Assert.Equal("0.105.0", torch.NormalizeNuGetVersion("0.105.0.0"));
Assert.Equal("0.1.0-alpha", torch.NormalizeNuGetVersion("0.1.0-alpha"));
Assert.Equal("0.1.0", torch.NormalizeNuGetVersion("0.1.0"));
Assert.Throws<ArgumentException>(() => NormalizeNuGetVersion(""));
Assert.Throws<ArgumentException>(() => NormalizeNuGetVersion("1.2.3.4.5"));
}

// Because some of the tests mess with global state, and are run in parallel, we need to
// acquire a lock before testing setting the default RNG see.
private static object _lock = new object();
Expand Down