Skip to content

Push classification caching to be an internal detail of semantic classification. #58326

Merged
CyrusNajmabadi merged 17 commits intodotnet:mainfrom
CyrusNajmabadi:classificationQueue
Dec 14, 2021
Merged

Push classification caching to be an internal detail of semantic classification. #58326
CyrusNajmabadi merged 17 commits intodotnet:mainfrom
CyrusNajmabadi:classificationQueue

Conversation

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

No description provided.

@CyrusNajmabadi CyrusNajmabadi requested review from a team as code owners December 14, 2021 04:02
@ghost ghost added the Area-IDE label Dec 14, 2021
TextSpan textSpan,
Checksum checksum,
StorageDatabase database,
CancellationToken cancellationToken);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

merged the semantic caching service with the semantic classification service.

if (_uniqueItems.Add(item))
_nextBatch.Add(item);
}
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this was just a move for clarity. This needs to be called under a lock. so i moved this into the only method that calls this to make sure no one else accidentally calls this incorrectly outsdie of hte lock.

}
}

private static async Task AddSemanticClassificationsAsync(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

all special logic about caching moved into the classification service itself.

@@ -1,77 +0,0 @@
// Licensed to the .NET Foundation under one or more agreements.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

no more incremental analysis to cause this work to happen. instead, we just use the pull requests on the OOP server to enqueue work to classify the full doc.


private static async Task<bool> TryGetCachedClassificationsAsync(
Document document, TextSpan textSpan, ArrayBuilder<ClassifiedSpan> result,
RemoteHostClient client, bool isFullyLoaded, CancellationToken cancellationToken)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

importantly, this work should not be pushed into the OOP client. the idea here is that we want to make this request without syncing over the full solution. so we must make the decision on the client to checked for cached results first, and if not go to normal results.

/// </summary>
internal interface IRemoteSemanticClassificationCacheService
{
ValueTask CacheSemanticClassificationsAsync(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this method is gone completely. the client does not choose when things are cached.

TextSpan textSpan,
Checksum checksum,
StorageDatabase database,
CancellationToken cancellationToken);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this method moved to the normal classification service.

@CyrusNajmabadi CyrusNajmabadi merged commit e77005a into dotnet:main Dec 14, 2021
@ghost ghost added this to the Next milestone Dec 14, 2021
@CyrusNajmabadi CyrusNajmabadi deleted the classificationQueue branch December 14, 2021 19:17
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

@allisonchou this is available for you now. Semantic classification caching should now work transparently.

@allisonchou
Copy link
Copy Markdown
Contributor

Awesome, thanks so much Cyrus!

@Cosifne Cosifne modified the milestones: Next, 17.1.P3 Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants