-
Notifications
You must be signed in to change notification settings - Fork 4
Adding ability to generate nuget packages #12
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
base: main
Are you sure you want to change the base?
Conversation
This will help solve asg017/sqlite-vec#193
The folders output in builds might not match the way we store RIDs in .NET, so we'll map them across
The package layout you sent me looks almost good, you're missing I'd also recommend matching version (the one you sent me is |
Co-authored-by: Krzysztof Wicher <[email protected]>
Hey this is awesome! I'm at a conference this weekend but will review this when I'm back, definitely want to get dotnet support here. Thank you again for setting this all up! |
@aaronpowell I believe there will be some tiny bit more to make that work on (old) .NET Framework - I'll try to figure out the details - we can still go ahead with this as is though and do this work in further iteration |
@aaronpowell I believe all we need is something equivalent to https://github.com/libgit2/libgit2sharp.nativebinaries/blob/master/nuget.package/build/net46/LibGit2Sharp.NativeBinaries.props |
so for netfx the simplest thing which works (in 64-bit) is: <?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<ItemGroup>
<Content Include="$(MSBuildThisFileDirectory)..\..\runtimes\win-x64\native\vec0.dll">
<Link>vec0.dll</Link>
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
<Pack>false</Pack>
</Content>
</ItemGroup>
</Project> under |
so it needs to include a props file in the |
@aaronpowell - I think it could be either. I tested with and some queries how other repos do it: |
I'll try to get more details from someone more knowledgeable in this, I'm not sure I yet fully understand what needs to happen exactly - I'm sure though that when I do #12 (comment) and name the file I guess we could also copy what https://www.nuget.org/packages/Microsoft.ML does which was given to me as the correct way to do it example - they have both props: <Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<!--
NuGet packages.config doesn't support native assemblies automatically,
so copy the native assemblies to the output directory.
-->
<ItemGroup Condition="Exists('packages.config') OR
Exists('$(MSBuildProjectName).packages.config') OR
Exists('packages.$(MSBuildProjectName).config')">
<Content Include="$(MSBuildThisFileDirectory)\..\..\runtimes\win-x64\native\*.dll"
Condition="'$(PlatformTarget)' == 'x64'">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
<Visible>false</Visible>
<Link>%(Filename)%(Extension)</Link>
</Content>
<Content Include="$(MSBuildThisFileDirectory)\..\..\runtimes\win-x86\native\*.dll"
Condition="'$(PlatformTarget)' == 'x86'">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
<Visible>false</Visible>
<Link>%(Filename)%(Extension)</Link>
</Content>
<Content Include="$(MSBuildThisFileDirectory)\..\..\runtimes\win-arm64\native\*.dll"
Condition="'$(PlatformTarget)' == 'arm64'">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
<Visible>false</Visible>
<Link>%(Filename)%(Extension)</Link>
</Content>
</ItemGroup>
</Project> and targets: <Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<PropertyGroup>
<EnableMLUnsupportedPlatformTargetCheck Condition="'$(EnableMLUnsupportedPlatformTargetCheck)' == ''">true</EnableMLUnsupportedPlatformTargetCheck>
</PropertyGroup>
<Target Name="_CheckForUnsupportedPlatformTarget"
Condition="'$(EnableMLUnsupportedPlatformTargetCheck)' == 'true'"
AfterTargets="_CheckForInvalidConfigurationAndPlatform">
<!--
Special case .NET Core portable applications. When building a portable .NET Core app,
the PlatformTarget is empty, and you don't know until runtime (i.e. which dotnet.exe)
what processor architecture will be used.
-->
<Error Condition="('$(PlatformTarget)' != 'x64' AND '$(PlatformTarget)' != 'x86') AND
('$(OutputType)' == 'Exe' OR '$(OutputType)'=='WinExe') AND
!('$(TargetFrameworkIdentifier)' == '.NETCoreApp' AND '$(PlatformTarget)' == '')"
Text="Microsoft.ML currently supports 'x64' and 'x86' processor architectures. Please ensure your application is targeting 'x64' or 'x86'." />
</Target>
</Project> they also put it under It seems this mechanism is needed when project uses package.config. |
@asg017 I don't want to bore you with above :-) TL;DR; we'll probably need Windows x86 asset to get this package to correctly work with most of the .NET versions because old ones use x86 by default. Do you think it would be feasible request? |
@aaronpowell I just validated the targets/props from ML.NET just work without modifications (minus error message when native asset is missing needs to be modified because it mentions ML.NET) - it also prints error message that asset is missing for this architecture (only on netfx though) |
This will enable support for netfx
I've update the PR to include generating the props and targets files. |
Co-authored-by: Krzysztof Wicher <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@asg017 this PR seems ready for merging - we're in the process of building samples for .NET and SQLite and would love to be able to show the streamlined experience! Will you be able to give this your attention in the next few days? Note that you'll also need to publish the package for consumption by .NET users, on nuget.org. We can help with that if you want. |
Hey @roji + @aaronpowell , thanks for all your work here! Definitely want to get this in. I checked out this branch and ran it with the new I'm not familiar with dotnet, so I have a few questions:
If you want to build this cargo run -- \
sample/sqlite-dist.toml \
--input sample/dist/ \
--output tmp \
--version 0.0.1 Then (apologies for the hackiness - I'll be documenting the Also - I'll look into getting x64 binaries built for sqlite-vec |
(to build the samples with zig, you may need |
Here's a super slimmed down version of how to use the extension NuGet package you created above @asg017. The only thing required would be a local install of .NET 8 (link for installers). This is the code: using Microsoft.Data.Sqlite;
var connectionString = "Data Source=:memory:";
using var connection = new SqliteConnection(connectionString);
connection.LoadExtension("sample0"); // load extension from NuGet package
connection.Open();
var command = connection.CreateCommand();
command.CommandText = "SELECT sample_version()";
var result = command.ExecuteScalar();
Console.WriteLine($"SQLite sample extension version: {result}"); The one thing you'll note is that the @krwq I wonder if we should have an overload on |
@aaronpowell the API is a thin shim over sqlite3_load_extension which will work with full path, path without extension or just a file name (which will search in its default directories). The fix I made to efcore made it also searches for directories with native binaries specific to .NET. But to answer your question you don't pass name anywhere but you can pass in name of the entry point in your extension with second optional argument. |
Thank you @aaronpowell for sharing! So in the other bindings, we have a pattern of exposing a programming language-level import sqlite3
import sqlite_vec
db = sqlite3.connect(":memory:")
db.enable_load_extension(True)
sqlite_vec.load(db) And JavaScript: import * as sqliteVec from "sqlite-vec";
import Database from "better-sqlite3";
const db = new Database(":memory:");
sqliteVec.load(db); And Ruby: require 'sqlite3'
require 'sqlite_vec'
db = SQLite3::Database.new(':memory:')
db.enable_load_extension(true)
SqliteVec.load(db) In other words, the Would it be possible to include an API like that in this? So something like: using Microsoft.Data.Sqlite;
using SqliteSample.Extension;
var connectionString = "Data Source=:memory:";
using var connection = new SqliteConnection(connectionString);
SqliteSample.load(connection); Not 100% sure if that's a valid package name to include. The current manual If that's not possible, maybe would could expose the libname as a variable? using Microsoft.Data.Sqlite;
using SqliteSample.Extension;
var connectionString = "Data Source=:memory:";
using var connection = new SqliteConnection(connectionString);
connection.LoadExtension(SqliteSample.name); // 'sample0' Again, not familiar with dotnet so open to other API suggestions! |
I think that in the .NET world, it could be reasonable to simply add an extension method over SqliteConnection to load sqlite_vec, so the user would write the following: using var connection = new SqliteConnection(connectionString);
connection.LoadSqliteVec();
// The extension method definition:
public static class SqliteConnectionExtensions
{
public static void LoadSqliteVec(this SqliteConnection connection)
=> connection.LoadExtension("sqlite_vec0"); // Or whatever the name of the binary is that the extension deploys
} Makes sense @krwq @aaronpowell? |
Yeah that'd work, but wouldn't we have to build a .NET binary on the CI of anything using this, thus adding some overhead to the consumer? Or maybe we could get away with a source only NuGet package but I've never tested that with runtimes in it like this (I guess it'd work) |
I'm not sure it's worth to add a library just to call I don't feel particularly strongly about having it or not but if we decide to have it then IMO extension method makes most sense and as @aaronpowell suggested I think this would make sense to be source-only package since dll is pretty heavy for just a single method. |
I'll admit I made a mistake in my assumption that the DLL name has to match the package ID, I thought it was doing some lookup through the referenced packages, but that's wrong, the runtime folders are copied to the output on build and it looks there. |
@krwq the main value I see in having the extension method is that the user doesn't have to embed a magic string (the name of the DLL), which is indeed not ideal... It's true that this seems heavy for an additional .NET DLL, but on the other hand that's presumably just an internal detail inside the nupkg that no user will actually need to know or care about (or am I missing something)? I definitely don't think that telling the user to install yet another source-only thing just for the extension is great - I'd either go with an in-the-box extension method in the same nupkg, or just drop the whole thing and tell them to call But I don't have any strong feelings here - @krwq @aaronpowell @asg017 I'm OK with whatever you think is best. |
I'll have a go at making it a source only package tomorrow |
Ah, didn't realize that adding a C# wrapper would require compiling a binary. Dont think it's worth it then, can always update later if needed. Let's go with this then - but one question about publishing: Which registry/registries should I submit the |
@asg017 yeah, the standard/public place for publishing .NET nuget packages is nuget.org - all .NET tools and IDEs look for packages there by default. You can set up a Github Action in your pipeline if you want (I can help with samples), but at least for the start you can also just manually upload your package via the web UI (it's super easy). Naming-wise you could call it SqliteVec (it's not taken), or if you want to give it some prefix (e.g. your name or something) you can (e.g. Once that's uploaded, anyone can start using the package by doing |
@asg017 you can use To get
|
I've tried to use the package provided by @aaronpowell to run Semantic Kernel tests. Thanks to help from @krwq I was able to get it working. FWIW the code is available here |
Great news @adamsitnik, thanks for confirming! FYI @krwq's change to Microsoft.Data.Sqlite went was backported to 9.0 and 8.0, so when then next patch version is out we can modify the Microsoft.Extensions.VectorData to use all this stuff (that's microsoft/semantic-kernel#11155, just assigned to you as you've already done the work ;)). |
@asg017 just checking in, are you having any trouble with nuget.org or similar? Would be great to see this merged and published soon as we'd like to take a dependency on it etc. |
Hey apologies for the delay - will take a 2nd look at this and hopefully merge in this week! |
Thank you @asg017! Please don't hesitate to ping us if you run into any trouble. |
Personally, I don't see a lot of value to this extension method. It doesn't make anything more obvious, you still need to read the docs to know you need to call it, and it doesn't save any work really. I believe a .NET-specific docs page would be sufficient here. Testing out the nupkg you linked, it broke my team's build by default because the extension class violates our code style rules. I don't think most .NET devs are aware that nupkg's can add source to a project, so I could see the presentation of this sort of error being pretty confusing. If you think it's important to include the extension, I'd suggest adding a comment at the top explaining that it came from the nupkg, and that the file can be excluded from your project by adding using |
I disagree. The following: connection.LoadSqliteVec(); ... seems more discoverable, less error-prone and generally nicer than this: connection.LoadExtension("sqlite_vec0"); LoadSqliteVec() can even be discovered via Intellisense - the moment one does
What kind of style rules are you using? LoadSqliteVec() is standard .NET naming.
I honestly don't see devs getting actually bit by this. We can always add a comment though. |
My team has many style rules that this file fails. For example, it's missing a copyright header and does not use file-scoped namespaces. But another team may have completely opposite style rules. You'd be surprised how many teams enforce style rules at build time. |
Thanks for the info @sandyarmstrong, that makes sense. A better way to deal with this may be to simply add @aaronpowell @krwq @adamsitnik what do you think? Is there some sort of better guidance for nuget-added source files? |
I don't have anything better than |
OK, I've made this suggestion which can be accepted to add @asg017 I think if you accept this suggestion everything should be ready for publishing the nuget. |
Co-authored-by: Shay Rojansky <[email protected]>
Change from @roji applied. I'm not able to generate a new nuget package for testing for a few more days, currently traveling and don't have access to the machine with everything setup for that. |
@asg017 I think we're good to go here - and time is really starting to get tight; hope you can merge this and publish the nuget soon! |
This will help solve asg017/sqlite-vec#193
I'm not 100% sure that the files will be in exactly the right place for NuGet, I need to review it in context of dotnet/efcore#35617 a bit more, but this was my first pass at writing some rust and trying to get it packaging a package.