-
Notifications
You must be signed in to change notification settings - Fork 783
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
Add MEAI.Evaluation libraries #5873
Conversation
...ies/Microsoft.Extensions.AI.Evaluation.Tests/Microsoft.Extensions.AI.Evaluation.Tests.csproj
Show resolved
Hide resolved
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=947607&view=codecoverage-tab |
src/Libraries/Microsoft.Extensions.AI.Evaluation.Console/Commands/CleanCacheCommand.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Evaluation.Console/Commands/CleanCacheCommand.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Evaluation.Console/Commands/CleanResultsCommand.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Evaluation.Console/Commands/CleanResultsCommand.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Evaluation.Console/Commands/CleanResultsCommand.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Evaluation/CompositeEvaluator.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Evaluation/EvaluatorExtensions.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Evaluation/Utilities/TaskExtensions.cs
Show resolved
Hide resolved
I rebased this on main after merging API changes, and added a commit here that adapts to those changes (just a few tweaks needed). |
…ationRequested calls
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=950078&view=codecoverage-tab |
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.
There are items we need to follow up on, but nothing that should block at this point in my opinion.
...Microsoft.Extensions.AI.Evaluation.Console/Microsoft.Extensions.AI.Evaluation.Console.csproj
Show resolved
Hide resolved
...xtensions.AI.Evaluation.Reporting/CSharp/Microsoft.Extensions.AI.Evaluation.Reporting.csproj
Show resolved
Hide resolved
...xtensions.AI.Evaluation.Reporting/CSharp/Microsoft.Extensions.AI.Evaluation.Reporting.csproj
Show resolved
Hide resolved
// To avoid infinite loops, ignore exceptions that were already seen. | ||
if (seen.Add(current)) |
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.
How would you get an infinite loop anyway? This method only loops for AggregateException.InnerExceptions, and I don't see how that could ever return a collection that includes the same AggregateException:
- AggregateException.InnerExceptions returns ReadOnlyCollection<Exception> so one cannot use that to modify the InnerExceptions collection of an existing AggregateException.
- AggregateException.InnerExceptions is not
virtual
so a class derived from AggregateException cannot override InnerExceptions to return something circular. - The AggregateException constructor copies the list of inner exceptions so AggregateException.InnerExceptions won't be affected if the caller later modifies the original list.
- C# does not allow the following so you cannot pass the AggregateException reference to its own constructor.
using System; public class MaliciousException : AggregateException { public MaliciousException() : base(innerExceptions: [this]) // error CS0027: Keyword 'this' is not available in the current context { } }
Exponential complexity would be possible though, if there are
Microsoft Reviewers: Open in CodeFlow