semantic-kernel icon indicating copy to clipboard operation
semantic-kernel copied to clipboard

Published 20 hours ago • verified publisher icon microsoft

Weaviate connector

Open codebrain opened this issue 1 year ago • 2 comments

Motivation and Context

Description

This adds a WeaviateMemoryStore, which allows for memories to be stored in Weaviate.

I have some outstanding questions, please help!

  1. Weaviate has strict naming conventions around class names (i.e. collections). What are the conventions in SK for class names?
  2. What are the most appropriate log levels for the connector? Please can you verify these are as expected?
  3. I have a suite of integration tests that can run against a Weaviate instance. I have implemented these integration tests before, in another project. Any guidance on how these should run for this repo? Ultimately it requires Weaviate running in a docker container.

Contribution Checklist

  • [x] The code builds clean without any errors or warnings
  • [x] The PR follows SK Contribution Guidelines (https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
  • [x] The code follows the .NET coding conventions (https://learn.microsoft.com/dotnet/csharp/fundamentals/coding-style/coding-conventions) verified with dotnet format
  • [x] All unit tests pass, and I have added new tests where possible
  • [x] I didn't break anyone :smile:

codebrain avatar May 25 '23 12:05 codebrain

Recreated from: https://github.com/microsoft/semantic-kernel/pull/738

Although I have now discovered, it is not possible to allow edits by maintainers, since the fork exists with in organisation.

codebrain avatar May 25 '23 12:05 codebrain

Ping: @lemillermicrosoft

codebrain avatar May 25 '23 12:05 codebrain

Still see some formatting issues and build issues. Waiting on that for further testing.

lemillermicrosoft avatar May 31 '23 18:05 lemillermicrosoft

Still see some formatting issues and build issues. Waiting on that for further testing.

Is there anything specifically I can do to have this merged

  • I have opened and compiled in VS and fixed the trailing whitespaces (see commit below).
  • I don't believe the build errors in the solution are as a result of the changes I have made, the errors are in files I have not changed, and in the file I did change I simply copy and pasted most of the implementation from the Qdrant connector. image

codebrain avatar Jun 01 '23 04:06 codebrain

Still see some formatting issues and build issues. Waiting on that for further testing.

Is there anything specifically I can do to have this merged

  • I have opened and compiled in VS and fixed the trailing whitespaces (see commit below).
  • I don't believe the build errors in the solution are as a result of the changes I have made, the errors are in files I have not changed, and in the file I did change I simply copy and pasted most of the implementation from the Qdrant connector. image

image

I think the other warnings are because the use of direct project reference causing conflicts with the nuget package references in copilotchatwebapi. I think you should remove that integration for now and leave as a separate follow up PR.

lemillermicrosoft avatar Jun 01 '23 15:06 lemillermicrosoft

Hi @dmytrostruk,

Thanks for your review. I've pushed back on some of your requested changes and provided reasoning. The other requested changes I have made here: https://github.com/microsoft/semantic-kernel/pull/1219/commits/68145b525d78e4871e40064b8afe058a837546df

Please take another look, thanks.

codebrain avatar Jun 02 '23 03:06 codebrain

@codebrain I think there are comments you skipped, since they are not resolved and you didn't comment them. They are hidden under this block, could you please take a look? Thank you! image

dmytrostruk avatar Jun 02 '23 11:06 dmytrostruk

@dmytrostruk - please can you review?

I have removed the SK prefix and wrapped every possible exception with the WeaviateMemoryException class. If this is the desired behaviour, then I would recommend that the other integrations are reviewed for consistency.

codebrain avatar Jun 05 '23 01:06 codebrain

Hi @dmytrostruk - I think given over a month of trying to get this PR in, its reasonable to expect I can make the proposed changes in a follow up PR? Don't you think?

codebrain avatar Jun 06 '23 11:06 codebrain

Hi @dmytrostruk - I think given over a month of trying to get this PR in, its reasonable to expect I can make the proposed changes in a follow up PR? Don't you think?

Hi @codebrain - sorry, I was not aware how much time did you spend on this functionality. I was requested to review this particular PR and that's what I'm trying to do :)

Regarding documentation and integration tests project structure, this can be done in follow-up PR.

About new warnings, this is first thing in our contribution checklist. In case you introduce new warnings, especially those which are easy to fix, you shouldn't expect merging it to main branch: image

The other thing is, for the warning problem, I added Suggested change section, so the warning can be fixed with one-click directly in PR. I would be happy to fix all the warnings and make other changes by myself to speed-up the process, but I don't have access to push changes to this PR for some reason: image

I hope that makes sense, thank you!

dmytrostruk avatar Jun 06 '23 12:06 dmytrostruk

@dluc please re-approve.

dmytrostruk avatar Jun 06 '23 12:06 dmytrostruk

About new warnings, this is first thing in our contribution checklist. In case you introduce new warnings, especially those which are easy to fix, you shouldn't expect merging it to main branch.

This is reasonable, however when I opened the PR originally, this was the case. I have been fighting underlying changes in the repository since then, an example being here, which changed AsyncEnumerable behaviour that I was dependant on.

At the same time as expecting contributors to adhere to the contributing guidelines it would be good if Microsoft was able to promptly review and suggest changes to open PRs, so that code changes are not left open for weeks on end. This is especially important on code bases that change rapidly - progress beats perfection

The other thing is, for the warning problem, I added Suggested change section, so the warning can be fixed with one-click directly in PR. I would be happy to fix all the warnings and make other changes by myself to speed-up the process, but I don't have access to push changes to this PR for some reason.

It was committed here. You likely don't have access as I do not work for Microsoft, I am not an approved committer and the fork exists in an organisation.

codebrain avatar Jun 06 '23 12:06 codebrain

It was committed here. You likely don't have access as I do not work for Microsoft, I am not an approved committer and the fork exists in an organisation.

Thanks. I was able to push directly into PRs from authors outside Microsoft, so maybe it's "fork in an organization" issue, or some policies were changed recently. Thanks again!

dmytrostruk avatar Jun 06 '23 12:06 dmytrostruk

Hi @dluc - I have had to resolve conflicts again in https://github.com/microsoft/semantic-kernel/pull/1219/commits/9dfd477705fb5dfd03aaf4d5e843e9b2be7a894e The example number needed to be bumped from 43 to 44.

When this branch goes green can it please be merged? Thanks.

codebrain avatar Jun 08 '23 08:06 codebrain

I'd like to merge this today but somehow github continues to report conflicts, resetting the approval flags

dluc avatar Jun 08 '23 17:06 dluc

Hi @dluc - I have had to resolve conflicts again in 9dfd477 The example number needed to be bumped from 43 to 44.

When this branch goes green can it please be merged? Thanks.

I've opened https://github.com/searchpioneer/semantic-kernel/pull/1 against searchpioneer to update this PR.

lemillermicrosoft avatar Jun 08 '23 19:06 lemillermicrosoft