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

Jump to bottom

.Net: MinHeap and TopNCollection should not be public #3152

Closed
Tracked by #3134
stephentoub opened this issue Oct 11, 2023 · 5 comments · Fixed by #3175
Closed
Tracked by #3134

.Net: MinHeap and TopNCollection should not be public #3152

stephentoub opened this issue Oct 11, 2023 · 5 comments · Fixed by #3175
Assignees
@dmytrostruk
Labels
.NET Issue or Pull requests regarding .NET code sk team issue A tag to denote issues that where created by the Semantic Kernel team (i.e., not the community)
Milestone
v1.0.0

Comments

@stephentoub
Copy link
Member

stephentoub commented Oct 11, 2023 •
edited by matthewbolanos
Loading

Important

Labeled Urgent because it may require a breaking change.

#615 was closed as completed with (I thought) the resolution to make MinHeap internal, but in the latest release it's still public. Both it and TopNCollection aren't something that are really in the domain of SK: a developer can use PriorityQueue<> in .NET, or OrderBy(...).Take(...), or any number of other solutions if they need the equivalent.
@shawncal shawncal added .NET Issue or Pull requests regarding .NET code triage labels Oct 11, 2023
@github-actions github-actions bot changed the title MinHeap and TopNCollection should not be public .Net: MinHeap and TopNCollection should not be public Oct 11, 2023
@matthewbolanos matthewbolanos mentioned this issue Oct 11, 2023
Closed
@matthewbolanos matthewbolanos added the sk team issue A tag to denote issues that where created by the Semantic Kernel team (i.e., not the community) label Oct 11, 2023
@dmytrostruk
Copy link
Member

@stephentoub We moved them to separate package Microsoft.SemanticKernel.Plugins.Memory and made them public as part of this package. One of the reasons is to be able to write some unit tests in separate test package. Otherwise, we will need to add InternalsVisibleTo attribute, and I'm not sure it's a good practice for testing internal classes/structures.

Do you have any recommendations here?

@dmytrostruk dmytrostruk self-assigned this Oct 11, 2023
@stephentoub
Copy link
Member Author

What are the unit tests doing with MinHeap/TopNCollection that they can't do with PriorityQueue, OrderBy.Take, etc.?

@dmytrostruk
Copy link
Member

What are the unit tests doing with MinHeap/TopNCollection that they can't do with PriorityQueue, OrderBy.Take, etc.?

Sorry, I meant unit tests for actual MinHeap/TopNCollection types :)
https://github.com/microsoft/semantic-kernel/blob/main/dotnet/src/Plugins/Plugins.UnitTests/Memory/Collections/MinHeapTests.cs
https://github.com/microsoft/semantic-kernel/blob/main/dotnet/src/Plugins/Plugins.UnitTests/Memory/Collections/TopNCollectionTests.cs

TopNCollecton is used in VolatileMemoryStore - our in-memory store implementation. Since Plugins.Memory package is based on .NET Standard 2.0, PriorityQueue is unavailable, unless we copy all its source code and place it in our repository.

@stephentoub
Copy link
Member Author

Sorry, I meant unit tests for actual MinHeap/TopNCollection types :)

Ok. Writing unit tests for internal functionality is not a good enough reason to make that functionality public. We should make them internal, and either a) use InternalsVisibleTo to test them directly, b) use reflection to test them directly, or c) stop testing them directly and instead test the actual functionality that is exposed and public.

TopNCollecton is used in VolatileMemoryStore

It doesn't need to be deleted. It just shouldn't be public.

@dmytrostruk
Copy link
Member

Makes sense, thank you! Will use InternalsVisibleTo flag then.

@matthewbolanos matthewbolanos added this to the v1.0.0 milestone Oct 12, 2023
@dmytrostruk dmytrostruk mentioned this issue Oct 13, 2023
Merged
4 tasks
@dmytrostruk dmytrostruk moved this to Sprint: In Review in Semantic Kernel Oct 13, 2023
github-merge-queue bot pushed a commit that referenced this issue Oct 16, 2023
@dmytrostruk
.Net: Hide Memory Collection types for internal usage only (#3175)
224f0db 
### Motivation and Context

<!-- Thank you for your contribution to the semantic-kernel repo!
Please help reviewers and future users, providing the following
information:
  1. Why is this change required?
  2. What problem does it solve?
  3. What scenario does it contribute to?
  4. If it fixes an open issue, please link to the issue here.
-->

Resolves: #3152

This PR contains changes to make Memory Collection types as `internal`.
As alternative, developers can use
[PriorityQueue](https://learn.microsoft.com/en-us/dotnet/api/system.collections.generic.priorityqueue-2?view=net-7.0)
type, available in .NET 6, 7, 8.

### Description

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->
1. Hide Memory Collection types.
2. Removed unit tests for hidden types, as they are covered with tests
for public type that uses them (`VolatileMemoryStore`).
### 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
- [ ] I didn't break anyone 😄
@github-project-automation github-project-automation bot moved this from Sprint: In Review to Sprint: Done in Semantic Kernel Oct 16, 2023
SOE-YoungS pushed a commit to SOE-YoungS/semantic-kernel that referenced this issue Nov 1, 2023
@dmytrostruk @SOE-YoungS
.Net: Hide Memory Collection types for internal usage only (microsoft…
ebdd60d 
…#3175)

### Motivation and Context

<!-- Thank you for your contribution to the semantic-kernel repo!
Please help reviewers and future users, providing the following
information:
  1. Why is this change required?
  2. What problem does it solve?
  3. What scenario does it contribute to?
  4. If it fixes an open issue, please link to the issue here.
-->

Resolves: microsoft#3152

This PR contains changes to make Memory Collection types as `internal`.
As alternative, developers can use
[PriorityQueue](https://learn.microsoft.com/en-us/dotnet/api/system.collections.generic.priorityqueue-2?view=net-7.0)
type, available in .NET 6, 7, 8.

### Description

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->
1. Hide Memory Collection types.
2. Removed unit tests for hidden types, as they are covered with tests
for public type that uses them (`VolatileMemoryStore`).
### 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
- [ ] I didn't break anyone 😄
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dmytrostruk dmytrostruk

Labels
.NET Issue or Pull requests regarding .NET code sk team issue A tag to denote issues that where created by the Semantic Kernel team (i.e., not the community)
Projects
Semantic Kernel
Archived in project
Milestone
v1.0.0
Development

Successfully merging a pull request may close this issue.

.Net: Hide Memory Collection types for internal usage only dmytrostruk/semantic-kernel
4 participants
@matthewbolanos @stephentoub @dmytrostruk @shawncal