Skip to content

.Net: Fix redis package reference#2985

Merged
dmytrostruk merged 9 commits intomicrosoft:mainfrom
JadynWong:jadyn/fix-redis
Sep 29, 2023
Merged

.Net: Fix redis package reference#2985
dmytrostruk merged 9 commits intomicrosoft:mainfrom
JadynWong:jadyn/fix-redis

Conversation

@JadynWong
Copy link
Contributor

Motivation and Context

Fix #2953

Description

Looks like, since #1799, there's been a problem. However, due to references from other projects, tests and samples have not shown the problem.

Contribution Checklist

@JadynWong JadynWong requested a review from a team as a code owner September 26, 2023 08:47
@shawncal shawncal added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel memory connector labels Sep 26, 2023
@JadynWong
Copy link
Contributor Author

Is there a better way to solve this problem?

Tested with a simple project, works fine with .net7.0 and .net8.0.

using Microsoft.SemanticKernel;
using Microsoft.SemanticKernel.Connectors.Memory.Redis;
using sk_redis_test;

var apiEndpoint = Env.Var("AzureOpenAI:Endpoint");
var apiKey = Env.Var("AzureOpenAI:ApiKey");
var kernelBuilder = new KernelBuilder();
kernelBuilder.WithAzureChatCompletionService("gpt-35-turbo", apiEndpoint, apiKey);
kernelBuilder.WithAzureTextEmbeddingGenerationService("text-embedding-ada-002", apiEndpoint, apiKey);
using var memoryStore = new RedisMemoryStore("localhost:6379");
kernelBuilder.WithMemoryStorage(memoryStore);
var kernel = kernelBuilder.Build();

var collectionName = "test";

for (int i = 1; i <= 10; i++)
{
    var key = await kernel.Memory.SaveInformationAsync(collectionName, i.ToString(), i.ToString(), i.ToString(), i.ToString());
    Console.WriteLine(key);
}

await memoryStore.DeleteCollectionAsync(collectionName);
<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net8.0</TargetFramework>
    <RootNamespace>sk_redis_test</RootNamespace>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
    <UserSecretsId>5ee045b0-aea3-4f08-8d31-32d1a6f8fed0</UserSecretsId>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.Extensions.Configuration.UserSecrets" />
  </ItemGroup>

  <ItemGroup>
    <ProjectReference Include="..\src\Connectors\Connectors.AI.OpenAI\Connectors.AI.OpenAI.csproj" />
    <ProjectReference Include="..\src\Connectors\Connectors.Memory.Redis\Connectors.Memory.Redis.csproj" />
  </ItemGroup>

</Project>

@dmytrostruk
Copy link
Member

@JadynWong Thanks for quick fix! Yes, we have similar problem in other packages as well. Looks like NRedisStack depends on System.Text.Json >= 7.0.2 which depends on Microsoft.Bcl.AsyncInterfaces >= 7.0.0, so there is version mismatch.

Is there a better way to solve this problem?

Usually, you can just specify concrete package version where it's needed, like this:

<PackageReference Include="Microsoft.Bcl.AsyncInterfaces" Version="7.0.0" />

But with central package management it's not allowed to specify version in PackageReference attribute, so we need to use PackageVersion instead:

<PackageVersion Update="Microsoft.Bcl.AsyncInterfaces" Version="7.0.0" />

We use similar approach in other existing packages as well.

@JadynWong Did you try to remove the line with package version update and verify that issue can be reproduced in your scenario with simple project .net7.0/.net8.0, and then when you add it back, the issue is resolved?

@JadynWong
Copy link
Contributor Author

JadynWong commented Sep 26, 2023

@dmytrostruk Yes. I reproduced the issue in the simple project using the same approach as you described. When adding Microsoft.Bcl.AsyncInterfaces and set the version to 7.0.0, the issue was solved.

@dmytrostruk
Copy link
Member

@dmytrostruk Yes. I reproduced the issue in the simple project using the same approach as you described. When adding Microsoft.Bcl.AsyncInterfaces and set the version to 7.0.0, the issue was solved.

@JadynWong Sounds awesome, thank you!

@dmytrostruk dmytrostruk added PR: ready for review All feedback addressed, ready for reviews PR: ready to merge PR has been approved by all reviewers, and is ready to merge. labels Sep 26, 2023
@dmytrostruk dmytrostruk added this pull request to the merge queue Sep 29, 2023
Merged via the queue into microsoft:main with commit 8c07296 Sep 29, 2023
@JadynWong JadynWong deleted the jadyn/fix-redis branch September 29, 2023 15:14
SOE-YoungS pushed a commit to SOE-YoungS/semantic-kernel that referenced this pull request Nov 1, 2023
### Motivation and Context
Fix microsoft#2953

### Description
Looks like, since microsoft#1799, there's been a problem. However, due to
references from other projects, tests and samples have not shown the
problem.

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [x] The code builds clean without any errors or warnings
- [x] The PR follows the [SK Contribution
Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [x] All unit tests pass, and I have added new tests where possible
- [x] I didn't break anyone 😄

Co-authored-by: Dmytro Struk <13853051+dmytrostruk@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kernel Issues or pull requests impacting the core kernel memory connector .NET Issue or Pull requests regarding .NET code PR: ready for review All feedback addressed, ready for reviews PR: ready to merge PR has been approved by all reviewers, and is ready to merge.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

.Net: Could not load file or assembly Microsoft.Bcl.AsyncInterfaces exception

4 participants